verif to smt conversion#549
Conversation
| def AggregateScalarizationPass : LLZKPass<"llzk-aggregate-scalarization"> { | ||
| let summary = | ||
| "Repeatedly scalarize array/pod aggregates until the module stabilizes"; | ||
| let description = [{ | ||
| Repeatedly runs the existing array-to-scalar and pod-to-scalar passes, | ||
| followed by canonicalization, until the module reaches a fixpoint. This is | ||
| intended as a reusable preprocessing step for pipelines that need | ||
| array/pod-free IR. | ||
| }]; | ||
| let options = []; | ||
| } |
There was a problem hiding this comment.
I've been thinking about this a bit and I think we can augment array-to-scalar to split arrays within a pod (just create N pod records, similar to how splitting an array-type struct member) so that a single pass of array-to-scalar then pod-to-scalar can remove all.
There was a problem hiding this comment.
Since that'll take a bit more effort, can we add that as a TODO and keep this solution for now?
There was a problem hiding this comment.
Update, Raghav needed all pod types removed without also removing array types so this PR increases the capabilities of pod-to-scalar such that running it should be able to remove all pod.type even within an array so pipelines can just run it before array-to-scalar.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1febe215c7
ℹ️ 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".
tim-hoffman
left a comment
There was a problem hiding this comment.
I have a few files to go through still but here's my feedback so far
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 375f7a624e
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab98bea16b
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38478589cd
ℹ️ 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".
0xddom
left a comment
There was a problem hiding this comment.
The verif to smt pass needs some work. A cleaner approach is to let the different dialects define how to lower to smt, similar to how lowering to LLVM works in upstream MLIR. The pass' name hints that is going to lower only operations from the verif dialect, but it lowers everything that is meant to be lowered to smt. The pass may probably be better renamed to -llzk-to-smt.
The current lowering logic is pretty fragile. If we add new ops (like bool.forall coming soon) then we need to change this pass as well to support it. If the implementation for converting from LLZK to SMT is defined by the ops or the dialect, then we only need to change the dialect or the op, a change probably closer to where the other changes that break the pass are happening.
The code as it stands right now is a good prototype for getting the demo working for yesterday but that's about it.
| @@ -201,7 +224,7 @@ class FunctionTypeConverter { | |||
| public: | |||
| virtual ~FunctionTypeConverter() = default; | |||
|
|
|||
| void convert(function::FuncDefOp op, mlir::RewriterBase &rewriter) { | |||
| template <typename FunctionLikeOp> void convert(FunctionLikeOp op, mlir::RewriterBase &rewriter) { | |||
There was a problem hiding this comment.
Change the docstring of the class now that it works on any function-like thing
| template <typename MatchType, typename NextMatchType, typename... MatchTypes, typename R> | ||
| inline static bool walkContains(R &root) { | ||
| return root | ||
| .walk([](mlir::Operation *op) { | ||
| return llvm::isa<MatchType, NextMatchType, MatchTypes...>(op) ? mlir::WalkResult::interrupt() |
There was a problem hiding this comment.
Why do you need typename MatchType, typename NextMatchType? That forces callers to check for at least 2 possibilities.
| static constexpr bool supportsResultAttrs() { | ||
| return requires(FunctionLikeOp op, ArrayAttr attrs) { | ||
| op.getResAttrsAttr(); | ||
| op.setResAttrsAttr(attrs); | ||
| }; |
There was a problem hiding this comment.
This pattern is used in LLZKConversionUtils.h as well so we could move the function there to share it.
| void rewrite(CallOp op, OpAdaptor adaptor, ConversionPatternRewriter &rewriter) const override { | ||
| // Create new CallOp with split results first so, then process its inputs to split types | ||
| CallOp newCall = newCallOpWithSplitResults(op, adaptor, rewriter); | ||
| rewriter.setInsertionPoint(newCall); |
There was a problem hiding this comment.
This is such that whatever is created next in processInputOperands is inserted before newCall, right? Don't we need to recover the previous insertion point after that? Otherwise it will continue inserting before the call unless the conversion engine saves the insertion point internally before calling a pattern's rewrite function.
| if (auto structDef = dyn_cast<StructDefOp>(op)) { | ||
| llvm::SmallVector<Type> memberTypes; | ||
| for (MemberDefOp member : structDef.getMemberDefs()) { | ||
| memberTypes.push_back(member.getType()); | ||
| } | ||
| if (failed(collectAggregateProfileForTypes(memberTypes, structDef, tables, profile))) { | ||
| return WalkResult::interrupt(); | ||
| } | ||
| } else if (auto funcDef = dyn_cast<FuncDefOp>(op)) { | ||
| llvm::SmallVector<Type> types; | ||
| llvm::append_range(types, funcDef.getArgumentTypes()); | ||
| llvm::append_range(types, funcDef.getResultTypes()); | ||
| if (failed(collectAggregateProfileForTypes(types, funcDef, tables, profile))) { | ||
| return WalkResult::interrupt(); | ||
| } | ||
| } else if (auto contract = dyn_cast<ContractOp>(op)) { | ||
| llvm::SmallVector<Type> types; | ||
| llvm::append_range(types, contract.getArgumentTypes()); | ||
| llvm::append_range(types, contract.getResultTypes()); | ||
| if (failed(collectAggregateProfileForTypes(types, contract, tables, profile))) { | ||
| return WalkResult::interrupt(); | ||
| } | ||
| } else if (auto createArray = dyn_cast<CreateArrayOp>(op)) { | ||
| if (failed(collectAggregateProfileForTypes( | ||
| ArrayRef<Type> {createArray.getType()}, createArray, tables, profile | ||
| ))) { | ||
| return WalkResult::interrupt(); | ||
| } | ||
| } else if (auto newPod = dyn_cast<NewPodOp>(op)) { | ||
| if (failed(collectAggregateProfileForTypes( | ||
| ArrayRef<Type> {newPod.getType()}, newPod, tables, profile | ||
| ))) { | ||
| return WalkResult::interrupt(); | ||
| } | ||
| } | ||
| return WalkResult::advance(); |
There was a problem hiding this comment.
You can use llvm::TypeSwitch here instead of chaining if-then-elses
| /// Convert a felt constant attribute into the equivalent SMT integer literal. | ||
| IntegerAttr toIntAttr(FeltConstantOp op) { | ||
| return IntegerAttr::get( | ||
| builder.getContext(), toAPSInt(toDynamicAPInt(op.getValue().getValue())) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The docstring and the function argument are at odds with each other. Is the function meant to take an op or an attribute?
| /// Lower signed division or remainder over field elements via signed representatives. | ||
| Value emitSignedDivOrRem(Location loc, Value lhs, Value rhs, FeltType type, bool isDiv) { | ||
| Value signedLhs = emitSignedRepresentative(loc, lhs, type); | ||
| Value signedRhs = emitSignedRepresentative(loc, rhs, type); | ||
| Value quotient = emitTruncatingSignedDivision(loc, signedLhs, signedRhs); | ||
| if (isDiv) { | ||
| return emitCanonical(loc, quotient, type); | ||
| } | ||
| Value product = | ||
| builder.create<smt::IntMulOp>(loc, ValueRange {signedRhs, quotient}).getResult(); | ||
| Value remainder = builder.create<smt::IntSubOp>(loc, signedLhs, product).getResult(); | ||
| return emitCanonical(loc, remainder, type); | ||
| } | ||
|
|
||
| /// Lower signed integer division over field elements. | ||
| Value emitSignedIntDivValue(Location loc, Value lhs, Value rhs, FeltType type) { | ||
| return emitSignedDivOrRem(loc, lhs, rhs, type, /*isDiv=*/true); | ||
| } | ||
|
|
||
| /// Lower signed remainder over field elements. | ||
| Value emitSignedModValue(Location loc, Value lhs, Value rhs, FeltType type) { | ||
| return emitSignedDivOrRem(loc, lhs, rhs, type, /*isDiv=*/false); | ||
| } |
There was a problem hiding this comment.
emitSignedDivOrRem doesn't seem to be used anywhere else so this looks over-complicated for no reason
| if (op == constrainFunc.getOperation() || isa<ReturnOp, scf::YieldOp>(op) || | ||
| isa<constrain::EmitEqualityOp>(op) || isa<CallOp>(op)) { |
There was a problem hiding this comment.
| if (op == constrainFunc.getOperation() || isa<ReturnOp, scf::YieldOp>(op) || | |
| isa<constrain::EmitEqualityOp>(op) || isa<CallOp>(op)) { | |
| if (op == constrainFunc.getOperation() || isa<ReturnOp, scf::YieldOp, constrain::EmitEqualityOp, CallOp>(op)) { |
There was a problem hiding this comment.
The organization in this file is confusing me
There was a problem hiding this comment.
Are we planning to support lowering the loop invariant ops in this PR?
Problem is we already have a lowering pass called that, but it's specifically for @Product aligned programs. We can rename that pass, perhaps, but that would require an LLEQ change. @raghav198 Otherwise I agree.
Correct. I think there are some PRs that actually need to be split off from this one and merged separately or removed. For example, there's a scalarization workaround that I believe needs to be reverted once #565 is merged. I'm just going to mark this as a draft until I'm done restructuring this, but I haven't had a chance to yet. |
verifcontracts tosmtveriftosmt