fix: register splitting of casted skip_if - #2039
fix: register splitting of casted skip_if#2039letypequividelespoubelles wants to merge 2 commits into
Conversation
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
There was a problem hiding this comment.
Pull request overview
Fixes ZkC VM register-splitting for skip_if when the comparison operands have different widths (commonly introduced by casts), by zero-extending the narrower side so vectorized comparisons have matching limb counts. This fits into the VM bytecode transform pipeline (pkg/zkc/vm/internal/transform) which normalizes/lowers bytecode to a split-register form compatible with the interpreter’s vectored branch encodings.
Changes:
- Update
splitSkipIfto handle mismatched operand limb counts by materializing a zero-extended, consecutively-indexed register vector. - Add a new ZkC unit fixture covering destructuring + casts feeding into
skip_ifcomparisons. - Register the new fixture in the ZkC unit test suite.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| testdata/zkc/unit/skip_if_06.zkc | New fixture exercising skip_if on casted/destructured values with differing widths. |
| testdata/zkc/unit/skip_if_06.accepts | Accepted traces for the new fixture. |
| pkg/zkc/vm/internal/transform/split_registers.go | Zero-extend narrower skip_if operands during register splitting; introduce zeroExtendLimbs. |
| pkg/test/zkc_unit_test.go | Add Test_ZkcUnit_SkipIf_06 to run the new unit fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DavePearce
left a comment
There was a problem hiding this comment.
Overall, I don't think this is the right fix. We can support SkipIf with variable sized operands, and the branch condition translator (see pkg/asm/compiler/branches.go) already handles this case correctly. In particular, since branch conditions compile down into polynomial constraints, its not efficient to allocate registers to handle differences in limb size.
Fix should be roughly like this:
- Remove check in
bytecode.NewSkipIfVec() - Fix encoding problem in encoding for
SkipIf - Update
executeSkipIf_rvto handle differing numbers of limbs - Possibly update
gogenaccordingly.
| // limbs and copying the original limbs across. | ||
| for i := range nlimbs { | ||
| if i < n-m { | ||
| nlimbs[i] = alloc.Allocate("z", util.Some[uint](0)) |
There was a problem hiding this comment.
You need to use alloc.ZeroRegister here. This helps to ensure only one zero register is every allocated, and that it is allocated in the correct form so we know its a zero registers.
No description provided.