Promote straight-line arrays before SROA - #736
Conversation
33093a3 to
99e8926
Compare
58caf55 to
18230cd
Compare
273febe to
4e6ddf0
Compare
4e6ddf0 to
ad708ab
Compare
| // Cleanup SSA values made dead by removing allocations and writes. | ||
| nestedPM.addPass(createRemoveDeadValuesWorkaroundPass()); | ||
| // Fold and remove local SSA values made dead by array promotion. Avoid the global | ||
| // remove-dead-values dataflow analysis here: static array lowering can produce very large, | ||
| // straight-line functions, and the targeted allocation cleanup above has already removed the | ||
| // memory state that required whole-region reasoning. Consequently, this pass no longer prunes | ||
| // dead function arguments/results or loop iteration values that canonicalization cannot remove. | ||
| nestedPM.addPass(mlir::createCanonicalizerPass()); |
There was a problem hiding this comment.
As discussed earlier...
Thus far, we have kept "canonicalize" out of standalone passes (only using it in pre-defined pipelines) because of how broad it is (potentially affecting any op in the IR, reordering, etc.). Since it's run at the very end here, it does not provide any additional power to this pass itself so we should remove it and add a comment to this pass declaration stating that running canonicalize after can be beneficial.
There was a problem hiding this comment.
@shankarapailoor This is the only feedback I have for now. This will of course change the test output and will require reverting the related part of the header comment in lib/Dialect/Array/Transforms/ArrayToScalarPass.cpp.
There was a problem hiding this comment.
@tim-hoffman Given the changes that will be there in LLVM 23, I think we decided to keep it here no?
ad708ab to
67c6bbf
Compare
67c6bbf to
041a26b
Compare
041a26b to
d4fcb64
Compare
Replace the global remove-dead-values analysis with canonicalization after array promotion. This avoids costly whole-region dataflow on large straight-line functions, but intentionally no longer guarantees pruning dead function arguments, function results, or loop iteration values that canonicalization cannot remove.
d4fcb64 to
c6b25d0
Compare
|
@codex review — Please focus on correctness: semantic preservation, miscompilations, edge cases, and regressions introduced by this PR. Flag concrete bugs with a triggering example or clear failure scenario. Avoid suggestions for alternative optimizations, performance improvements, refactoring, or style unless they are necessary to fix a correctness issue. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| // CHECK-DAG: %[[V11:[0-9a-zA-Z_\.]+]] = struct.readm %[[V0]][@f1_6] : <@ComponentA1>, !felt.type | ||
| // CHECK-DAG: %[[V12:[0-9a-zA-Z_\.]+]] = struct.readm %[[V0]][@f1_0] : <@ComponentA1>, !felt.type | ||
| // CHECK-DAG: %[[V13:[0-9a-zA-Z_\.]+]] = struct.readm %[[V0]][@f1_1] : <@ComponentA1>, !felt.type | ||
| // CHECK-NOT: struct.readm |
There was a problem hiding this comment.
The 3 CHECK-NOT added to this file are redundant in a chain of CHECK-NEXT
| // CHECK-LABEL: function.def @inside_loop_body | ||
| // CHECK-NOT: array. | ||
| // CHECK: scf.for |
There was a problem hiding this comment.
This doesn't seem meaningful to check since there were no array. prior to scf.for originally. Is the pass able to remove the array usage in the loop body or not?
There was a problem hiding this comment.
This is why I typically prefer CHECK for the full output.
| // straight-line functions, and the targeted allocation cleanup above has already removed the | ||
| // memory state that required whole-region reasoning. Consequently, this pass no longer prunes | ||
| // dead function arguments/results or loop iteration values that canonicalization cannot remove. | ||
| nestedPM.addPass(mlir::createCanonicalizerPass()); |
There was a problem hiding this comment.
You don't need the mlir:: prefix since using namespace mlir is in the file
Regenerate affected FileCheck blocks with generate-test-checks.py, cover the full loop body, and remove the redundant mlir namespace qualifier. Co-authored-by: Codex <codex@openai.com>
2 participants
Summary
This is PR 2 of 3 in the R1CS lowering stack.
Stack:
Speed up array-to-scalar lowering by directly promoting eligible straight-line static array elements before the general SROA and mem2reg pipeline. Replace the expensive global dead-value analysis with targeted allocation cleanup and canonicalization.
Related issues
No linked issue.
Changes
scf.execute_regionoperations while rejecting CFG backedges and other multi-block paths.Testing
nix develop --command bash -c "cmake --build build --target check-lit"nix build -LSubmission checklist
AI assistance
Tools used: OpenAI Codex and Anthropic Claude.
How the tools contributed: Codex assisted with implementation, focused tests, performance-oriented restructuring, and validation. Claude independently reviewed CFG and aliasing assumptions and supplied adversarial reproducers.
How I verified the contribution: I reviewed the diffs, tested positive and negative
scf.execute_regionshapes, ran the complete lit suite, and completed a full Nix release build with unit tests.