fix(mem2reg): Consider aliases of a loaded address to be loaded from as well

[aakoshh]

Description

Problem*

Resolves #9538

Summary*

Fixes mem2reg to add the aliases of the address in a Load instruction to the last_loads of the PerFunctionContext.

Additional Context

In #9305 I added handling of IfElse instructions to mem2reg where if the result of the IfElse was a reference, then the then_value and the else_value were considered to be aliases of that result address. For example consider this SSA in the new integration test:

After Simplifying (4) (step 31):
brillig(inline) predicate_pure fn main f0 {
  b0(v0: u1, v1: [u8; 3], v2: [u8; 3]):
    v4 = allocate -> &mut [u8; 3]
    store v1 at v4
    v5 = allocate -> &mut [u8; 3]
    store v2 at v5
    v6 = not v0
    jmpif v0 then: b1, else: b2
  b1():
    v9 = not v0
    v10 = if v0 then v5 else (if v9) v4
    v11 = load v10 -> [u8; 3]                     	// src/main.nr:5:9
    inc_rc v11                                    	// src/main.nr:5:9
    jmp b3(v11)
  b2():
    v7 = if v0 then v5 else (if v6) v4            	// src/main.nr:3:9
    v8 = load v7 -> [u8; 3]                       	// src/main.nr:5:9
    inc_rc v8                                     	// src/main.nr:5:9
    jmp b3(v8)
  b3(v3: [u8; 3]):
    return v3
}

In v10 = if v0 then v5 else (if v9) v4, v5 and v4 are registered as aliases of v10.

In #9305 this had the effect in the handling of Call: for example if v10 was passed to a call, then all of its aliases would be added to instruction_references. In this bug, however, that was not enough.

The problem was we removed the stores to v5 and v4. The final decision whether to keep them looks like this:

                if !self.last_loads.contains_key(store_address)
                    && !store_alias_used
                    && !is_dereference
                {
                    self.instructions_to_remove.insert(*store_instruction);
                }

I focused a lot on store_alias_used, but that doesn't apply here: when we consider whether to remove the store to v4, it doesn't have an alias that is used, instead, v4 is an alias of v10. I'm not sure why this relationship isn't bidirectional, but it looked to me like last_loads is the key here: normally we would be loading v4 or v5, but instead we are loading them indirectly due to the IfElse. I thought it makes sense that if what we load has aliases, we might consider them being loaded as well, which directly prevents stores to them from being removed.

Conveniently last_loads acts across blocks, while Block::last_store does not, so even though Load marks the address as used, which marks all of its aliases as used, which removes the last_store from being considered for removal, it doesn't affect the previous Block, where it will remain a candidate and be removed anyway.

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.

[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: 785df98	Previous: 4451b45	Ratio
test_report_zkpassport_noir-ecdsa_	2 s	1 s	2

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

CC: @TomAFrench

[aakoshh]

To replicate the test failure in https://github.com/noir-lang/noir/actions/runs/17078806369/job/48427962743?pr=9567 :

❯ cargo run -q -p nargo_cli -- debug --force-brillig --expression-width 3
[regression_9538] Starting debugger
Debugger ready to receive messages..
At opcode 0:0 :: BRILLIG CALL func 0: inputs: [EXPR [ (1, _0) 0 ], [EXPR [ (1, _1) 0 ], EXPR [ (1, _2) 0 ], EXPR [ (1, _3) 0 ]], [EXPR [ (1, _4) 0 ], EXPR [ (1, _5) 0 ], EXPR [ (1, _6) 0 ]]], outputs: [[_7, _8, _9]]
> continue
(Continuing execution...)
The application panicked (crashed).
Message:  assertion failed: field_as_bytes.into_iter().all(|b| b == 0)
Location: /Users/aakoshh/Work/aztec/noir/compiler/noirc_printable_type/src/lib.rs:403

[asterite]

Looks good!

I'm not sure why this relationship isn't bidirectional

This is something I also was wondering while auditing the mem2reg code. The fix here is good but I wonder if there's a different way to code mem2reg to keep the alias set in a more straight-forward way.

[vezenovm]

Looks good, the description makes sense.