Skip to content

Fix nested member aux rebuild#501

Open
Kuhai9801 wants to merge 13 commits into
project-llzk:mainfrom
Kuhai9801:fix-nested-member-aux-rebuild
Open

Fix nested member aux rebuild#501
Kuhai9801 wants to merge 13 commits into
project-llzk:mainfrom
Kuhai9801:fix-nested-member-aux-rebuild

Conversation

@Kuhai9801

Copy link
Copy Markdown
Contributor

Summary

Fix compute-side auxiliary expression reconstruction for nested member reads.

rebuildExprInCompute previously rebuilt every MemberReadOp from the enclosing compute function's %self. That preserved the member name but discarded the original read component, so a constrain-side path such as self.mod2.val could be rebuilt in compute() as self.val.

This updates the helper to:

  • map constrain %self to compute %self
  • recursively rebuild the original MemberReadOp component
  • create the rebuilt read from that preserved component path

Fixes #491.

Testing

Updated the existing poly-lowering FileCheck case with both Mod1.@val and Mod2.@val.

The test now requires compute-side aux reconstruction to read:

%child = struct.readm %self[@mod2]
%child_val = struct.readm %child[@val]

instead of incorrectly reading direct Mod1.@val.

@Kuhai9801 Kuhai9801 requested a review from a team as a code owner June 1, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes auxiliary-expression reconstruction during poly/R1CS lowering so that compute-side witness code preserves the full nested struct.readm component path (e.g., self.mod2.val stays self.mod2.val instead of becoming self.val), addressing issue #491.

Changes:

  • Update rebuildExprInCompute to map constrain %self to compute %self and to recursively rebuild MemberReadOp components.
  • Strengthen the existing poly-lowering FileCheck to require compute-side reconstruction via @mod2 then @val (nested member read).
  • Add an unreleased changelog entry describing the fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
test/Transforms/PolyLowering/poly_lowering_pass_deg2.llzk Updates FileCheck expectations to require nested member-read reconstruction in compute().
lib/Transforms/LLZKLoweringUtils.cpp Fixes expression rebuild logic for nested MemberReadOp and %self mapping.
changelogs/unreleased/fix-nested-member-aux-rebuild.yaml Documents the fix in the unreleased changelog.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/Transforms/LLZKLoweringUtils.cpp
Comment thread lib/Transforms/LLZKLoweringUtils.cpp
Comment on lines -196 to +201
// CHECK: %[[VAL_3:.*]] = struct.readm %[[VAL_2]][@val] : <@Mod1>, !felt.type
// CHECK: %[[VAL_4:.*]] = felt.mul %[[VAL_3]], %[[VAL_3]] : !felt.type, !felt.type
// CHECK: struct.writem %[[VAL_2]][@__llzk_poly_lowering_pass_aux_member_1] = %[[VAL_4]] : <@Mod1>, !felt.type
// CHECK: %[[VAL_5:.*]] = felt.mul %[[VAL_0]], %[[VAL_1]] : !felt.type, !felt.type
// CHECK: struct.writem %[[VAL_2]][@__llzk_poly_lowering_pass_aux_member_2] = %[[VAL_5]] : <@Mod1>, !felt.type
// CHECK: %[[CHILD:.*]] = struct.readm %[[VAL_2]][@mod2] : <@Mod1>, !struct.type<@Mod2>
// CHECK: %[[CHILD_VAL:.*]] = struct.readm %[[CHILD]][@val] : <@Mod2>, !felt.type
// CHECK: %[[AUX_VAL:.*]] = felt.mul %[[CHILD_VAL]], %[[CHILD_VAL]] : !felt.type, !felt.type
// CHECK: struct.writem %[[VAL_2]][@__llzk_poly_lowering_pass_aux_member_1] = %[[AUX_VAL]] : <@Mod1>, !felt.type
// CHECK: %[[ARG_PRODUCT:.*]] = felt.mul %[[VAL_0]], %[[VAL_1]] : !felt.type, !felt.type
// CHECK: struct.writem %[[VAL_2]][@__llzk_poly_lowering_pass_aux_member_2] = %[[ARG_PRODUCT]] : <@Mod1>, !felt.type

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shankarapailoor can you verify this change is correct?

@tim-hoffman

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85e870438b

ℹ️ 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".

Comment thread lib/Transforms/LLZKLoweringUtils.cpp Outdated
SmallVector<Value> rebuiltGroup;
rebuiltGroup.reserve(group.size());
for (Value operand : group) {
rebuiltGroup.push_back(rebuildExprInCompute(operand, computeFunc, builder, memo));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard call map operand rebuilding for nested block args

When a rebuilt call's affine-map operands include an scf induction/block argument, this recursion sends that region BlockArgument through the function-argument mapping above; an induction variable has argument number 0, so it is replaced with computeFunc.getSelfValueFromCompute() rather than an index value. For calls like function.call ... {(%i)} inside a loop, the compute-side clone is emitted at the function terminator with a struct value in the map operand list, producing invalid IR or wrong instantiation instead of preserving the loop/index context.

Useful? React with 👍 / 👎.

@Kuhai9801

Copy link
Copy Markdown
Contributor Author

I wanted to check the intended behavior for poly/R1CS lowering here. Should aux expressions that depend on nested-region block args, like scf.for IVs or iter_args, be supported?

rebuildExprInCompute currently maps any BlockArgument by ordinal to compute args, which seems correct for top-level constrain args but not for nested-region args. If that case is unsupported for now, I think it would be better to reject it explicitly with a diagnostic and add a regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Poly lowering rebuilds nested member-read auxiliary expressions from the wrong component

3 participants