Skip to content

Commit

Permalink
fix(ssa refactor): Fix stack overflow during loop unrolling (#1666)
Browse files Browse the repository at this point in the history
Fix stack overflow during loop unrolling
  • Loading branch information
jfecher committed Jun 15, 2023
1 parent 4233068 commit c7a7216
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 7 deletions.
4 changes: 3 additions & 1 deletion crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ impl Instruction {
match self {
Instruction::Binary(binary) => binary.simplify(dfg),
Instruction::Cast(value, typ) => {
if let Some(value) = (*typ == dfg.type_of_value(*value)).then_some(*value) {
if let Some(constant) = dfg.get_numeric_constant(*value) {
SimplifiedTo(dfg.make_constant(constant, typ.clone()))
} else if let Some(value) = (*typ == dfg.type_of_value(*value)).then_some(*value) {
SimplifiedTo(value)
} else {
None
Expand Down
14 changes: 8 additions & 6 deletions crates/noirc_evaluator/src/ssa_refactor/opt/unrolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,10 @@ fn find_all_loops(function: &Function) -> Loops {
}
}

// Sort loops by block size so that we unroll the smaller, nested loops first as an optimization.
loops.sort_by(|loop_a, loop_b| loop_b.blocks.len().cmp(&loop_a.blocks.len()));
// Sort loops by block size so that we unroll the larger, outer loops of nested loops first.
// This is needed because inner loops may use the induction variable from their outer loops in
// their loop range.
loops.sort_by_key(|loop_| loop_.blocks.len());

Loops {
failed_to_unroll: HashSet::new(),
Expand Down Expand Up @@ -160,7 +162,7 @@ fn unroll_loop(function: &mut Function, cfg: &ControlFlowGraph, loop_: &Loop) ->
let mut unroll_into = get_pre_header(cfg, loop_);
let mut jump_value = get_induction_variable(function, unroll_into)?;

while let Some(context) = unroll_loop_header(function, loop_, unroll_into, jump_value) {
while let Some(context) = unroll_loop_header(function, loop_, unroll_into, jump_value)? {
let (last_block, last_value) = context.unroll_loop_iteration();
unroll_into = last_block;
jump_value = last_value;
Expand Down Expand Up @@ -213,7 +215,7 @@ fn unroll_loop_header<'a>(
loop_: &'a Loop,
unroll_into: BasicBlockId,
induction_value: ValueId,
) -> Option<LoopIteration<'a>> {
) -> Result<Option<LoopIteration<'a>>, ()> {
// We insert into a fresh block first and move instructions into the unroll_into block later
// only once we verify the jmpif instruction has a constant condition. If it does not, we can
// just discard this fresh block and leave the loop unmodified.
Expand Down Expand Up @@ -241,11 +243,11 @@ fn unroll_loop_header<'a>(
// unroll_into block from now on.
context.insert_block = unroll_into;

loop_.blocks.contains(&context.source_block).then_some(context)
Ok(loop_.blocks.contains(&context.source_block).then_some(context))
} else {
// If this case is reached the loop either uses non-constant indices or we need
// another pass, such as mem2reg to resolve them to constants.
None
Err(())
}
}
other => unreachable!("Expected loop header to terminate in a JmpIf to the loop body, but found {other:?} instead"),
Expand Down

0 comments on commit c7a7216

Please sign in to comment.