Skip to content

Commit

Permalink
fix: Move remove_if_else pass after second inlining (#4976)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

For the test added in this PR I bumped the `max_iter` in
`try_merge_only_changed_indices`. We then get the following panic:
```
The application panicked (crashed).
Message:  internal error: entered unreachable code: All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value v41 = Instruction { instruction: Id(76), position: 0, typ: Array([Numeric(Unsigned { bit_size: 32 })], 30) }
Location: compiler/noirc_evaluator/src/ssa/opt/inlining.rs:253
``` 
We have to inline immediately following mem2reg before extra
instructions are added by the remove_if_else pass.

## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
vezenovm committed May 3, 2024
1 parent a87c655 commit 96fb3e9
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 2 deletions.
4 changes: 3 additions & 1 deletion compiler/noirc_evaluator/src/ssa.rs
Expand Up @@ -59,10 +59,12 @@ pub(crate) fn optimize_into_acir(
.run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:")
// Run mem2reg once more with the flattened CFG to catch any remaining loads/stores
.run_pass(Ssa::mem2reg, "After Mem2Reg:")
.run_pass(Ssa::remove_if_else, "After Remove IfElse:")
// Run the inlining pass again to handle functions with `InlineType::NoPredicates`.
// Before flattening is run, we treat functions marked with the `InlineType::NoPredicates` as an entry point.
// This pass must come immediately following `mem2reg` as the succeeding passes
// may create an SSA which inlining fails to handle.
.run_pass(Ssa::inline_functions_with_no_predicates, "After Inlining:")
.run_pass(Ssa::remove_if_else, "After Remove IfElse:")
.run_pass(Ssa::fold_constants, "After Constant Folding:")
.run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:")
.run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:")
Expand Down
Expand Up @@ -306,7 +306,7 @@ impl<'a> ValueMerger<'a> {

// Arbitrarily limit this to looking at at most 10 past ArraySet operations.
// If there are more than that, we assume 2 completely separate arrays are being merged.
let max_iters = 1;
let max_iters = 2;
let mut seen_then = Vec::with_capacity(max_iters);
let mut seen_else = Vec::with_capacity(max_iters);

Expand Down
@@ -0,0 +1,7 @@
[package]
name = "array_if_cond_simple"
type = "bin"
authors = [""]
compiler_version = ">=0.28.0"

[dependencies]
@@ -0,0 +1,2 @@
x = true
y = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
@@ -0,0 +1,8 @@
fn main(x: bool, mut y: [u32; 30]) {
if x {
y[0] = 1;
}

let z = y[0] + y[1];
assert(z == 1);
}

0 comments on commit 96fb3e9

Please sign in to comment.