feat(ssa): Basic control dependent LICM #7660
Draft
+490
−83
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…als between the current block and the pre-header, can be made more advanced
![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: fbebe85 | Previous: 6eaea18 | Ratio |
|---|---|---|---|
noir-lang_noir_bigcurve_ |
261 s |
199 s |
1.31 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
5 tasks
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
… to avoid repeat work, allow control dependence checks on acir again
There was a problem hiding this comment.
Compilation Time
| Benchmark suite | Current: 096dcb3 | Previous: 9983007 | Ratio |
|---|---|---|---|
regression_4709 |
0.698 s |
0.715 s |
0.98 |
ram_blowup_regression |
14.8 s |
15 s |
0.99 |
global_var_regression_entry_points |
0.48 s |
0.497 s |
0.97 |
private-kernel-inner |
2.312 s |
2.314 s |
1.00 |
private-kernel-reset |
7.126 s |
6.86 s |
1.04 |
private-kernel-tail |
1.2 s |
1.206 s |
1.00 |
rollup-base-private |
15.72 s |
14.8 s |
1.06 |
rollup-base-public |
11.08 s |
10.9 s |
1.02 |
rollup-block-root-empty |
0.918 s |
0.933 s |
0.98 |
rollup-block-root-single-tx |
120 s |
125 s |
0.96 |
rollup-block-root |
123 s |
128 s |
0.96 |
rollup-merge |
0.91 s |
0.9 s |
1.01 |
rollup-root |
1.486 s |
1.498 s |
0.99 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Time
| Benchmark suite | Current: 096dcb3 | Previous: 9983007 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.069 s |
0.068 s |
1.01 |
private-kernel-reset |
0.284 s |
0.285 s |
1.00 |
private-kernel-tail |
0.026 s |
0.027 s |
0.96 |
rollup-base-private |
0.734 s |
0.728 s |
1.01 |
rollup-base-public |
0.484 s |
0.482 s |
1.00 |
rollup-block-root |
17 s |
16.9 s |
1.01 |
rollup-merge |
0.006 s |
0.006 s |
1 |
rollup-root |
0.024 s |
0.024 s |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
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: 60a01ba | Previous: 4c1c76e | Ratio |
|---|---|---|---|
regression_4709 |
0.893 s |
0.693 s |
1.29 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
Test Suite Duration
| Benchmark suite | Current: 096dcb3 | Previous: 9983007 | Ratio |
|---|---|---|---|
AztecProtocol_aztec-packages_noir-projects_aztec-nr |
41 s |
41 s |
1 |
AztecProtocol_aztec-packages_noir-projects_noir-contracts |
75 s |
76 s |
0.99 |
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
48 s |
42 s |
1.14 |
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
170 s |
167 s |
1.02 |
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib |
10 s |
10 s |
1 |
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib |
166 s |
157 s |
1.06 |
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
54 s |
54 s |
1 |
noir-lang_noir-bignum_ |
79 s |
85 s |
0.93 |
noir-lang_noir_bigcurve_ |
209 s |
209 s |
1 |
noir-lang_noir_json_parser_ |
9 s |
8 s |
1.13 |
noir-lang_sha512_ |
23 s |
25 s |
0.92 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Compilation Memory
| Benchmark suite | Current: 096dcb3 | Previous: 9983007 | Ratio |
|---|---|---|---|
private-kernel-inner |
299.97 MB |
300.04 MB |
1.00 |
private-kernel-reset |
609.94 MB |
609.94 MB |
1 |
private-kernel-tail |
226.62 MB |
226.64 MB |
1.00 |
rollup-base-private |
1250 MB |
1250 MB |
1 |
rollup-base-public |
1330 MB |
1330 MB |
1 |
rollup-block-root-empty |
303.45 MB |
303.44 MB |
1.00 |
rollup-block-root-single-tx |
7860 MB |
7860 MB |
1 |
rollup-block-root |
7870 MB |
7870 MB |
1 |
rollup-merge |
301.87 MB |
301.88 MB |
1.00 |
rollup-root |
349.43 MB |
349.4 MB |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Memory
| Benchmark suite | Current: 096dcb3 | Previous: 9983007 | Ratio |
|---|---|---|---|
private-kernel-inner |
239.85 MB |
239.85 MB |
1 |
private-kernel-reset |
274.25 MB |
274.25 MB |
1 |
private-kernel-tail |
213.48 MB |
213.48 MB |
1 |
rollup-base-private |
507.63 MB |
507.63 MB |
1 |
rollup-base-public |
416.42 MB |
416.42 MB |
1 |
rollup-block-root |
1420 MB |
1420 MB |
1 |
rollup-merge |
290.45 MB |
290.45 MB |
1 |
rollup-root |
296.92 MB |
296.92 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
…l dep blocks after the header not the preheader
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Resolves #7569
This PR resolves the optimizations described in the issue. However, the optimization can be expanded by further accounting for predicates and possibly even hoisting instructions under predicates as part of loop peeling/splitting. I leave these tasks to be captured in separate issues.
Summary*
We have certain instructions that we mark as unsafe for hoisting when they are only dependent upon a predicate and do not have any other side effects (e.g. a checked arithmetic operation). You can look into the linked issue for an example of logic that will not be hoisted.
If a predicated instruction shares the same predicate as the entire loop we can now hoist this instruction. The new unit tests under
loop_invariant.rsprovide examples of when a predicated instruction can now be hoisted.We check for whether an instruction shares a predicate with the entire loop through control dependence analysis. If the block containing an instruction is control dependent on any blocks between the pre-header and the instruction's block we know that there has been some branching. We then mark the current block as being control dependent.
If a block is not control dependent, we mark instructions that are reliant upon predicates (but do not have other side effects) as safe for hoisting.
We determine whether a block is control dependent on a parent block by following Definition 3 in Ferrante et al., 1987, The program dependence graph and its use in optimization.
Builds upon #7595 to utilize the post-dominator tree. We can further optimize our control dependence check by looking at the post-dominator frontiers (which we will refer to as reverse dominance frontiers).
As per, Morgan, B. (97) Building an Optimizing Compiler, the dominance frontier DF(B) of a block B is the set of all blocks C such that B dominates a predecessor of C but either B equals C or B does not dominate C.
Switching the above statement from dominance to post-dominance, we can see that the conditions for control dependence are the same as that of post-dominance frontiers. In fact, we can rewrite our control dependence definition from above to the following: Y is control dependent on X iff Y is in PDF(X).
If we were to check the control dependence as defined in Ferrante et al., 1987, The program dependence graph and its use in optimization, this would require n^2 in the worst case for checking whether a given loop block is control dependent on any block between it and the loop pre header. Generating post-dominance frontiers allows us to move to a linear worst case control dependency check.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.