chore: don't optimize ArrayGet from previous ArraySet#11586
chore: don't optimize ArrayGet from previous ArraySet#11586
Conversation
![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 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 0acc2d1 | Previous: efc4e62 | Ratio |
|---|---|---|---|
perfectly_parallel_batch_inversion_opcodes |
2724388 ns/iter (± 1394) |
2203755 ns/iter (± 8009) |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 9810fea | Previous: bd5f9f0 | Ratio |
|---|---|---|---|
semaphore_depth_10 |
188.85 MB |
98.09 MB |
1.93 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 6028306 | Previous: 32d579f | Ratio |
|---|---|---|---|
rollup-checkpoint-root |
12.6 s |
10.3 s |
1.22 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 0acc2d1 | Previous: efc4e62 | Ratio |
|---|---|---|---|
rollup-tx-base-public |
98.28 s |
80.14 s |
1.23 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Opcode count'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: 9810fea | Previous: bd5f9f0 | Ratio |
|---|---|---|---|
semaphore-depth-10 |
8045 opcodes |
5699 opcodes |
1.41 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
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: 26d937e | Previous: 460c017 | Ratio |
|---|---|---|---|
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 9a5dd41 | Previous: 32d579f | Ratio |
|---|---|---|---|
rollup-tx-merge |
0.002 s |
0.001 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
Ah interesting. I was hoping that we didn't rely on this as much. Of we can't solve this with an ssa pass then we can gove the simplification logic knowledge about whether the cfg is flattened. |
|
I got it working! That said, I'd like to comment the code or refactor it because there are some duplications... well, not exactly duplications but codes that are similar. |
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Elaboration Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 1391771 | Previous: cca6747 | Ratio |
|---|---|---|---|
rollup-root |
1.676 s |
1.352 s |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/value_merger.rs
Outdated
Show resolved
Hide resolved
aakoshh
left a comment
aakoshh
left a comment
There was a problem hiding this comment.
Looks great, just some nits 👏
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
3 participants
Description
Problem
Fixes a potential issue in the way we are currently simplifying
array_getby looking at previous instructions.See #11529 (comment)
Summary
Before this PR, an
array_getwith a constant index was simplified to the value set at the same index of a previousarray_set. However, this is only okay to do if thearray_setis under the same side effects var as thearray_get.This PR then does several things:
array_getsimplification doesn't do the above anymore. However, it will still look througharray_setwith a constant index that's different than the index of thearray_get, which could eventually lead to amake_arrayor param, to perform a simplificationarray_set, in order to do the original optimization. It usually runs after mem2reg.RemoveIfElseoptimization, where we use aValueMerger, we now keep track of side effects vars associated to eacharray_set. In that way when arrays are merged we can be sure we are doing this optimization correctly. This is essentially running the optimization in point 2 while merging arrays inRemoveIfElseAdditional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.