Arithmetize constraint-side scf.if operations - #643
Conversation
ef3b102 to
e174691
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
The PCL conversion pass shouldn't care about what goes on inside |
0xddom
left a comment
0xddom
left a comment
There was a problem hiding this comment.
Still going through the code. I have a first batch of comments.
| if (failed(validateBranch(ifOp, ifOp.getThenRegion().front()))) { | ||
| return failure(); | ||
| } | ||
| if (!ifOp.getElseRegion().empty() && failed(validateBranch(ifOp, ifOp.getElseRegion().front()))) { | ||
| return failure(); | ||
| } | ||
| return success(); |
There was a problem hiding this comment.
success and failure accept a bool as optional argument so this could be written as something similar to this:
| if (failed(validateBranch(ifOp, ifOp.getThenRegion().front()))) { | |
| return failure(); | |
| } | |
| if (!ifOp.getElseRegion().empty() && failed(validateBranch(ifOp, ifOp.getElseRegion().front()))) { | |
| return failure(); | |
| } | |
| return success(); | |
| return failure( | |
| failed(validateBranch(ifOp, ifOp.getThenRegion().front())) || | |
| (!ifOp.getElseRegion().empty() && failed(validateBranch(ifOp, ifOp.getElseRegion().front()))) | |
| ); |
| continue; | ||
| } | ||
| if (auto eq = llvm::dyn_cast<EmitEqualityOp>(op)) { | ||
| if (!llvm::isa<FeltType>(eq.getLhs().getType())) { |
There was a problem hiding this comment.
Is not necessarily the case that eq.getLhs().getType() == eq.getRhs().getType() so this check should probably handle both.
| op.emitError() << "cannot arithmetize scf.if branch operation '" << op.getName() | ||
| << "' because executing both branches may change its effects"; | ||
| return failure(); |
There was a problem hiding this comment.
The result of this expression can be converted into a LogicalResult.
| op.emitError() << "cannot arithmetize scf.if branch operation '" << op.getName() | |
| << "' because executing both branches may change its effects"; | |
| return failure(); | |
| return op.emitError() << "cannot arithmetize scf.if branch operation '" << op.getName() | |
| << "' because executing both branches may change its effects"; |
There are other places where this could be applied as well.
| for (mlir::scf::IfOp ifOp : ifOps) { | ||
| if (failed(validate(ifOp))) { | ||
| return failure(); | ||
| } | ||
| } |
There was a problem hiding this comment.
You can use llvm::any_of for this. Something like
if (llvm::any_of(ifOps, [this](mlir::scf::IfOp ifOp) { return failed(validate(ifOp)); })) {
return failure();
}
| }]; | ||
| } | ||
|
|
||
| def ArithmetizeSCFIfPass : LLZKPass<"llzk-arithmetize-scf-if"> { |
There was a problem hiding this comment.
We can also make this pass an op pass for function.def. The same rationale in #640 applies here.
|
Built this branch and ran it on a real Noir Minor, and pre-existing (not introduced here): with the pass built but not invoked, |
4 participants
Implements stage 3 of #585.
Adds
-llzk-arithmetize-scf-if, which replaces constraint-sidescf.ifoperations with select polynomials. Felt results useelse + condition * (then - else), while branch constraints are gated so inactive branches do not apply. Nested conditionals are supported.Because both branches execute after rewriting, branch operations must be safe to speculate. Unsupported operations and non-felt results or constraints are rejected. Run
-llzk-algebraize-felt-opsfirst for felt division and non-native operations.Also fixes PCL legalization for operations outside constraint functions, such as
scfoperations in@compute.Tested with the ArithmetizeSCFIf and PCLLowering lit suites. Together with #586 and #640, the full pipeline lowers all eight
noir_llzkexamples, includingmulti_scalar_mul.