fix(ssa): Track RC instructions when removing last loads

[vezenovm]

Description

Problem*

Resolves #6674

Summary*

This brings back code that was removed in #6019, but now for repeated loads rather than trivial stores. When analyzing instructions we check whether the last instruction was an inc_rc or a dec_rc. If it was, we do not remove the repeated load.

Needing to bring this back is very strange to me. Repeat loads are essentially "trivial loads" and an extra inc rc instruction as in #6674 (comment) should just cause our array to be copied rather than mutated. We should continue to try and get a small reproduction of the issue to understand it better and bring it into our test suite.

Another option is to simply revert #6088

Additional Context

Important information from the issue (#6674 (comment))

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]

Changes to Brillig bytecode sizes

Generated at commit: 2c1bce9cd8268223ae4d50f81a8265b78e0487e1, compared to commit: 31640e91ba75b9c5200ea66d1f54cc5185e0d196

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
nested_array_dynamic	+1 ❌	+0.04%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
nested_array_dynamic	2,779 (+1)	+0.04%

[github-actions]

Peak Memory Sample

Program	Peak Memory
keccak256	81.46M
workspace	122.26M
regression_4709	333.68M
ram_blowup_regression	2.55G

[github-actions]

Changes to number of Brillig opcodes executed

Generated at commit: 2c1bce9cd8268223ae4d50f81a8265b78e0487e1, compared to commit: 31640e91ba75b9c5200ea66d1f54cc5185e0d196

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
nested_array_dynamic	+1 ❌	+0.03%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
nested_array_dynamic	3,721 (+1)	+0.03%

[jfecher]

Needing to bring this back is very strange to me. Repeat loads are essentially "trivial loads" and an extra inc rc instruction should just cause our array to be copied rather than mutated.

This is strange to me as well. Of course copying versus mutating can be a breaking change but in the linked issue the two inc_rc instructions should be to the same array in either case. Agreed on needing to investigate further with getting a reproduceable example as well.

I was testing something locally with the array_refcount intrinsic and found an interesting case where the equivalent of

v0 = make_array [FIeld 0]
inc_rc v0
v1 = call array_refcount(v0)
return v1

Was returning 0 somehow. The actual example had another inc_rc after a printout of the first, and the second was zero as well. I wonder if these cases are related to something odd going on with ref counts behind the scenes.

[vezenovm]

This PR looks to now be causing private_kernel_tail_to_public::tests::enqueued_public_calls_consume_startup_gas to fail...

I'm continuing to investigate.

[vezenovm]

This PR looks to now be causing private_kernel_tail_to_public::tests::enqueued_public_calls_consume_startup_gas to fail...

@asterite mentioned that reverting #6088 also causes this test to fail...

[jfecher]

Was returning 0 somehow. The actual example had another inc_rc after a printout of the first, and the second was zero as well. I wonder if these cases are related to something odd going on with ref counts behind the scenes.

Sorry, I forgot the context of what I was looking at. The issue there was that DIE was removing RCs it shouldn't be, specifically the very first one. It's probably not related to this issue after all.

[vezenovm]

This PR looks to now be causing private_kernel_tail_to_public::tests::enqueued_public_calls_consume_startup_gas to fail...

Both tests are passing when this PR is combined with #6585. I've pushed draft of the two merged here #6692

[jfecher]

Sanity check: do we still need this at all after #6585 ?

If so, I'm happy merging this now and creating an issue for later to investigate why the extra load is needed.

[vezenovm]

Sanity check: do we still need this at all after #6585 ?

If so, I'm happy merging this now and creating an issue for later to investigate why the extra load is needed.

It isn't needed after #6585. We probably can close it, as this PR is a bit of a hack and shouldn't be necessary if we are laying down the correct instructions. It looks to provide an ~10% improvement in the uhashmap execution trace, but with slight increases in bytecode size and execution trace for every other program it affects.

[jfecher]

It looks to provide an ~10% improvement in the uhashmap execution trace

Ahh, another really odd result. Wish I knew why that was the case too.

Anyway, I'll leave this to you on whether we should merge this or not

[vezenovm]

I'm going to close this most likely but merging master to see its affect after all the RC changes.

[vezenovm]

Now we get no improvements from doing this, which is as expected