chore(mem2reg): removing seemingly unnecessary code#9616
chore(mem2reg): removing seemingly unnecessary code#9616
Conversation
asterite
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 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: 0decd4d | Previous: 3c6914c | Ratio |
|---|---|---|---|
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| self.set_aliases(references, array, aliases.clone()); | ||
| aliases | ||
| } else { | ||
| AliasSet::unknown() |
There was a problem hiding this comment.
Removing this whole section for aliases of arrayset instructions scares me haha.
I suppose it's indicative we don't have tests for it at the very least - I'd hesitate to say it's completely unnecessary but it does have me doubting.
There was a problem hiding this comment.
I agree. At least some part of this PR must not be right because Aztec-Packages failed: http://ci.aztec-labs.com/bf55284bb1e49ea4
Now I'm checking if it also fails in the currently open PR for the noir sync (it's failing for another reason, but maybe the failure is before this PR).
|
This breaks Aztec-Packages so I'm closing this. I'll try to see if I can find a small failing test for this. |
|
Huh, it seems that the latest Noir sync bumps into the same failure this PR bumped into: AztecProtocol/aztec-packages#16548 That likely means that the changes here aren't the failure cause. I'll try it again once the noir sync is fixed because I'm curious to see if this breaks anything. (in any case if we go with the |
2 participants
Description
Problem
Resolves #9394
Summary
I don't fully intend this to get merged but it's interesting that there seems to be a lot of code that, when removed, has all tests still pass. Maybe there's some code that fails but isn't tested here? If CI passes I'll try to see if syncing this with Aztec-Packages causes no failures. If failures happen maybe we can craft some tests to prove why the code was needed.
Additional Context
Documentation
Check one:
PR Checklist
cargo fmton default settings.