Fuse matching product scf.if regions#484
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4213c67d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| auto hasInternalRead = | ||
| constrainIf->walk([](MemberReadOp) { return mlir::WalkResult::interrupt(); }); |
There was a problem hiding this comment.
Reject constrain calls that can read delayed writes
When the constrain branch contains a function.call (for example a nested @constrain(%self, ...)) and a member written between computeIf and constrainIf is read inside that callee, this check still allows fusion because it only rejects direct MemberReadOps in the region. The fused branch clones the call before the intervening struct.writem operations that are left after the new scf.if, so the callee can observe stale/unwritten member state compared with the original ordering.
Useful? React with 👍 / 👎.
raghav198
left a comment
There was a problem hiding this comment.
The constrain use is remapped to the branch-local compute value instead of hoisting the read.
I may be misunderstanding, but I think this is not safe when the struct.readm is a constrain op:
%0 = felt.add %a, %b {source = "compute"}
struct.writem %self[@foo] = %0 {source = "compute"}
%foo = struct.readm %self[@foo] {source = "constrain"}
constrain.eq %foo, %c {source = "constrain"}
emits a constraint guaranteeing that the @foo signal equals the result of %c, whereas in the remapped
%0 = felt.add %a, %b {source = "compute"}
struct.writem %self[@foo] = %0 {source = "compute"}
constrain.eq %0, %c {source = "constrain"}
the emitted constraint doesn't refer to a struct signal at all.
|
Thanks for catching this, Raghav. You're right. Remapping the struct.readm result to the branch local compute value can remove the member signal from the emitted constraint. I'll make this fusion case conservative for now and add a regression test so we keep the constraint tied to the member read. |
There was a problem hiding this comment.
Pull request overview
This PR extends the -llzk-fuse-product-loops pass to also fuse matching compute/constrain scf.if regions in struct product programs (Fixes #300), and adds regression tests covering fusable and non-fusable scf.if patterns.
Changes:
- Add
scf.iffusion logic that clones compute + constrain branches into a single fusedscf.ifwhen conditions and structural constraints match. - Refactor pass entry/recursion to fuse both
scf.ifandscf.forcontrol flow via a sharedfuseMatchingRegionControlFlowroutine. - Add lit/FileCheck tests for
scf.iffusion and “do not fuse” guardrails; add an unreleased changelog entry.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Transforms/LLZKFuseProductLoopsPass.cpp | Implements scf.if fusion and refactors recursion over region control flow. |
| test/Transforms/FuseProductLoops/fuse_if_no_member_read.llzk | Positive test ensuring scf.if fusion occurs in a safe case. |
| test/Transforms/FuseProductLoops/fuse_loop_then_if.llzk | Ensures loop fusion still works and enables nested scf.if fusion after loop fusion. |
| test/Transforms/FuseProductLoops/no_fuse_if_call_after_write.llzk | Negative test: blocks fusion when a call occurs after a mapped write. |
| test/Transforms/FuseProductLoops/no_fuse_if_condition_mismatch.llzk | Negative test: blocks fusion when conditions differ. |
| test/Transforms/FuseProductLoops/no_fuse_if_internal_read.llzk | Negative test: blocks fusion when constrain side reads members inside the scf.if. |
| test/Transforms/FuseProductLoops/no_fuse_if_read_after_write.llzk | Negative test: blocks fusion when a member read appears between the ifs. |
| changelogs/unreleased/fuse-product-if-regions.yaml | Changelog entry for the new scf.if fusion capability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| body.walk<mlir::WalkOrder::PreOrder>([&witnessLoops, &constraintLoops](mlir::scf::ForOp forOp) { | ||
| if (!forOp->hasAttrOfType<mlir::StringAttr>(PRODUCT_SOURCE)) { | ||
| std::optional<llvm::StringRef> productSource = getProductSource(forOp); | ||
| if (!productSource) { | ||
| return mlir::WalkResult::skip(); | ||
| } | ||
| auto productSource = forOp->getAttrOfType<mlir::StringAttr>(PRODUCT_SOURCE); | ||
| if (productSource == FUNC_NAME_COMPUTE) { | ||
| if (*productSource == FUNC_NAME_COMPUTE) { | ||
| witnessLoops.push_back(forOp); | ||
| } else if (productSource == FUNC_NAME_CONSTRAIN) { | ||
| } else if (*productSource == FUNC_NAME_CONSTRAIN) { | ||
| constraintLoops.push_back(forOp); |
| } | ||
| hasInterveningWrite = true; | ||
| continue; | ||
| } | ||
|
|
||
| if (llvm::isa<MemberReadOp>(op)) { | ||
| // Replacing a member read with the branch-local computed value can remove the member signal | ||
| // from emitted constraints. Keep this fusion conservative until reads can be preserved. | ||
| return false; | ||
| } |
Summary
Fixes #300.
Adds conservative
scf.iffusion to-llzk-fuse-product-loopsfor matching compute/constrain product regions.The fusion handles narrow cases where matching compute and constrain
scf.ifregions have the same SSA condition and can be fused without changing member read/write ordering. It rejects constrain-side member reads and calls after mapped writes, so member reads remain tied to their original storage semantics until a more precise remapping analysis is added.Testing
git diff --checknix --extra-experimental-features "nix-command flakes" build -L .#debugClangllzk-opt | FileCheckchecks for:test/Transforms/FuseProductLoops/fuse_if.llzktest/Transforms/FuseProductLoops/no_fuse_if_condition_mismatch.llzktest/Transforms/FuseProductLoops/no_fuse_if_internal_read.llzk