fix(ssa): Simplify instructions during remove_unreachable_instructions

[aakoshh]

Description

Problem*

Resolves #9695

Summary*

Changed SimpleOptimizationContext to check the hash of the instruction before and after the optimisation pass, and if changes are detected then work like FunctionInserter::push_instruction_value and use DataFlowGraph::insert_instruction_and_results_if_simplified, instead of just pushing the instruction ID to the end of the block, which results in the instruction potentially be simplified out of the SSA.

This is relied on by remove_unreachable_instructions: when we encounter a conditional failure, which triggers the UnreachableUnderPredicate mode that replaces side-effecting instructions with default values, those values can be constants that can now enable simplifications for subsequent instructions.

Additional Context

In the bug ticket we had this SSA:

    enable_side_effects v2
    v5 = div Field 1, Field 0                     	
    v6 = div Field 1, Field 0                     	
    v7 = truncate v6 to 128 bits, max_bit_size: 254	

The pass recognised that v5 will always fail if v2 is true, and inserted a conditional constraint. That caused v6 to be replaced by a default 0 (which doesn't appear in the SSA), and subsequently caused the constant u64 0 to appear in v7:

    v5 = div Field 1, Field 0                     	
    constrain u1 0 == v2, "attempt to divide by zero"	
    v7 = truncate Field 0 to 128 bits, max_bit_size: 254	

By the time we get to ACIR gen, we don't expect truncate to operate on constants, however the simplify module does handle it.

Instead of adding special casing during ACIR gen, I thought it would be nicer to make remove_unreachable_instruction work akin to flatten_cfg, which simplifies during insertion.

With this change the SSA simplifies as follows:

After Remove Unreachable Instructions (2) (step 41):
acir(inline) impure fn main f0 {
  b0(v0: Field):
    v2 = eq v0, Field -1                          	
    enable_side_effects v2                        	
    v5 = div Field 1, Field 0                     	
    constrain u1 0 == v2, "attempt to divide by zero"
    constrain v2 == u1 0                          	
    enable_side_effects u1 1                      	
    return
}

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 and/or cargo fmt on default settings.

[asterite]

When I was auditing mem2reg I realized simple_optimization didn't simplify instructions like FunctionInserter. I tried to change simple_optimization to always simplify instructions if possible, that is, unconditionally, but it led to some failures. I wonder if it would be possible to always do it, it might be simpler than having to decide to do it or not.

Another thing I tried is to only simplify if an instruction would change because some of its values changed. That is, Instruction::replace_values returned a bool that said whether any value changed. If not, we can insert it without simplifying because if it didn't change then there's no way it will be simplified (I think!).

[aakoshh]

I tried to change simple_optimization to always simplify instructions if possible, that is, unconditionally, but it led to some failures.

Interesting, do you remember what failed? I assume you tracked the new IDs.

I didn't think it was necessary to unconditionally simplify instructions, it seems like a lot of work relative to what is happening now, and that's work that might not even have to be done. I thought this would be the lightest touch, but according to the reports it might already have increased the memory requirements.

That reminds me that there is an option to keep the existing instruction if it wasn't simplified; pushed a commit to take advantage of that.

Another thing I tried is to only simplify if an instruction would change because some of its values changed. That is, Instruction::replace_values returned a bool that said whether any value changed.

That seems reasonable. The only thing I'm not sure about is that simple_optimization_result calls replace_values, so yes, if there was a change we know we can try to simplify, but then the lambda can also modify the instruction in place, before it's inserted at the end, and those modifications might mean that something that didn't have to be simplified to begin with can become eligible by the end.

[aakoshh]

I have an idea: we can hash the values in the instruction before and after the call to see if we need to simplify. I'll try that.

[aakoshh]

@asterite I tried to make simplification automatic, but it's not entirely safe to do so; I ended up putting it behind a flag, but now it uses the hash to check if it needs to do it for the given instruction. The flag is set to true by remove_unreachable_instructions, but it's done unconditionally, so there is less thinking about where it has to be set, reset, etc.

The reason it couldn't be done automatically is that the static_assert_empty_loop integration test failed. Consider this Noir code:

    let x: [Field; 0] = [];
    let _ = x.as_slice().map(|_| {
        std::static_assert(false, "test");
        0
    });

So we know that the array is empty, and therefore map will not run, and the static_assert should not fail. But with simplification it did! This was the SSA before it:

After `as_slice` optimization (1) (step 11):
acir(inline) fn main f0 {
  b0():
 	  ...
    v6 = make_array [] : [Field]                  	
    v7 = allocate -> &mut u32                     	
    store u32 0 at v7                             	
    v9 = allocate -> &mut [Field]                 	
    store v6 at v9                                	
    ...
    jmp b1(u32 0)
  b1(v0: u32):
    v30 = lt v0, u32 0                            	
    jmpif v30 then: b2, else: b3
  b2():
    ...                    	
    constrain u1 0 == u1 1, "test"                	
    ...  	
    call static_assert(u1 0, v14, v29, u1 0)      	
    ...             	
    v43 = unchecked_add v0, u32 1                 	
    jmp b1(v43)
  b3():
    return
}

And this is with the simplification:

After `as_slice` optimization (1) (step 11):
acir(inline) fn main f0 {
  b0():
    ...
    jmp b1(u32 0)
  b1(v0: u32):
    jmpif u1 0 then: b2, else: b3
  b2():
    ...
    v41 = unchecked_add v0, u32 1                 	
    jmp b1(v41)
  b3():
    return
}

Notice how the terminator of b1 had v30 replaced by u1 0, so it always go to b3. Why does it fail, then?
That's because the static assertion pass looks at the loop boundaries to find blocks which are in empty loops, and that assumes having an induction variable there, not a constant, so it appears as a non-empty loop.

I'll have a look to see if we can enhance loop detection for this, but I had trouble with it before during unrolling.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: cf5bb77	Previous: 8fd9446	Ratio
test_report_zkpassport_noir-ecdsa_	3 s	2 s	1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[aakoshh]

Okay so now the simple optimisation loop simplifies any instruction that changed, and unrolling does some extra work to restore loop bounds which were simplified out.

A potential downside is that e.g. the remove_if_else pass, which is anyway a bit complicated, now has an output which is yet another step removed from the input. We considered disabling the simplification for this pass, or just its test, but decided to keep the number of free variables simple and just accept that the SSA changes more. After all, flattening and mem2reg, and whatever else uses the FunctionInserter, also simplify stuff.