perf: REM/DIV with constant + /% new operation - #2105
perf: REM/DIV with constant + /% new operation#2105letypequividelespoubelles wants to merge 22 commits into
/% new operation#2105Conversation
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
There was a problem hiding this comment.
Pull request overview
This PR improves ZkC VM execution and lowering performance by (1) introducing dedicated constant-divisor DIV/REM bytecodes (DIVC/REMC) and (2) merging complementary DIV/REM lowering blocks when both operations use identical operands, while also extending intrinsic source operands to support constants.
Changes:
- Add DIVC/REMC (and wide variants) for division/remainder by constant divisors, including encoding/decoding and interpreter support.
- Enhance division lowering to optionally merge adjacent complementary DIV/REM over the same operands into a single
DIV_HINT+ validation block. - Generalize intrinsic sources from register vectors to typed operands (register vectors or constants), and update splitting, inlining, lowering, encoding, interpreter, and codegen accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/zkc/vm/transform.go | Removes the exported OptimizeDivisions wrapper. |
| pkg/zkc/vm/internal/transform/split/shift.go | Updates wide shift intrinsic construction to use operand-typed sources. |
| pkg/zkc/vm/internal/transform/split/divrem.go | Adds constant-aware operand splitting/materialisation for divisor and emits const DivRem where applicable. |
| pkg/zkc/vm/internal/transform/split_registers.go | Splits intrinsic operands (including constant materialisation) instead of only register vectors. |
| pkg/zkc/vm/internal/transform/program_to_program.go | Updates cross-word conversion to preserve operand-typed divisors and intrinsic sources. |
| pkg/zkc/vm/internal/transform/optimize_division.go | Deletes the former fast-mode power-of-two division optimization pass. |
| pkg/zkc/vm/internal/transform/lower_division.go | Supports constant divisors in division lowering and merges complementary DIV/REM pairs into one lowering block. |
| pkg/zkc/vm/internal/transform/inline_functions.go | Updates inlining substitutions for operand-typed DivRem divisors and intrinsic sources. |
| pkg/zkc/vm/internal/interpreter/interpreter.go | Adds DIVC/REMC execution; updates intrinsic execution/packing to handle operand-typed sources and constant-pool access. |
| pkg/zkc/vm/internal/interpreter/encoding/iter.go | Adds flagged operand decoding (NextOperand) for constant-vs-register operand pairs. |
| pkg/zkc/vm/internal/interpreter/encoding/div.go | Adds DIVC/REMC encoding; updates intrinsic encoding to support constant operands via flagged lengths. |
| pkg/zkc/vm/internal/interpreter/encoding/common.go | Introduces new opcode constants for DIVC/REMC and wide variants; wires them into the main encoder. |
| pkg/zkc/vm/internal/gogen/emit_field.go | Updates intrinsic operand assembly/packing to support constant operands. |
| pkg/zkc/vm/internal/gogen/emit_bitwise.go | Updates DivRem emission to allow constant divisors. |
| pkg/zkc/vm/internal/bytecode/operand.go | Adjusts constant operand string formatting (hex prefix). |
| pkg/zkc/vm/internal/bytecode/intrinsic.go | Changes intrinsic sources from register vectors to operands (register vector or constant) and updates Uses/String. |
| pkg/zkc/vm/internal/bytecode/div.go | Changes DivRem divisor from register id to operand (register or constant) and updates validation/uses/stringing. |
| pkg/zkc/vm/internal/bytecode/bytecode.go | Adds constructors for constant-divisor DivRem. |
| pkg/zkc/vm/bytecode.go | Exposes new API helpers for DivConst / RemConst and updates Intrinsic API signature. |
| pkg/zkc/compiler/codegen/statement.go | Emits constant-divisor Rem/Div bytecodes (and folds constant divisors for division chains). |
| pkg/zkc/compiler/codegen/compile.go | Removes the now-deleted OptimizeDivisions pass and updates transform comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: François Bojarski <54240434+letypequividelespoubelles@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (4)
pkg/zkc/compiler/codegen/statement.go:742
- The "division by zero" syntax error is currently attributed to args[0] (the dividend), but the error is caused by the divisor(s). Pointing at the first divisor expression provides a more accurate source location in diagnostics.
if hasConst && product.Sign() == 0 {
p.errors = append(p.errors, p.srcmaps.SyntaxErrors(args[0], "division by zero")...)
}
pkg/zkc/vm/internal/bytecode/intrinsic.go:98
- The comment references split.SplitOperand, but the helper introduced for this behavior is split.Operand. Updating the reference avoids confusion during maintenance.
// Constant sources must be single-limb: their limb widths are not
// recorded, so a multi-limb run could be neither encoded nor reconstructed
// (see split.SplitOperand, which materialises over-wide constants into
// registers instead).
testdata/zkc/unit/div_05.zkc:3
- This unit test exercises a runtime-dependent divisor (8 vs 16). Without the removed header comment, it’s hard to see that it’s a regression case ensuring division-by-constant lowering doesn’t incorrectly treat the divisor as statically known.
pub input data(address:u8) -> (byte:u8)
fn main() {
pkg/zkc/vm/internal/bytecode/intrinsic.go:57
- This comment says intrinsic sources can be "possibly multi-limb" constants, but Intrinsic.Validate (and encoding) only supports single-limb constant operands. Updating the comment avoids misleading future changes.
// Sources are the arguments read by this intrinsic, each either a register
// vector or a (possibly multi-limb) constant.
Sources []Operand[W]
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
pkg/zkc/vm/internal/transform/split_registers.go:537
- Comment references
split.SplitOperand, but the helper is namedsplit.Operandin this package. Updating the reference avoids confusion when navigating the code.
// splitOperandVectors splits each operand (e.g. an intrinsic argument) into
// limbs: register vectors into the limbs of their constituent registers,
// whilst constants stay single-limb — those too wide for one limb are
// materialised into registers via the returned load bytecodes, which must
// precede the consuming instruction (see split.SplitOperand).
pkg/zkc/compiler/codegen/statement.go:743
- The division-by-zero error is attached to
args[0](the dividend), which will report the wrong source location for expressions likea / 0(and likely break the expected//error:... division by zerofixtures). Prefer attributing the error to the zero divisor expression.
if hasConst && product.Sign() == 0 {
p.errors = append(p.errors, p.srcmaps.SyntaxErrors(args[0], "division by zero")...)
}
DavePearce
left a comment
There was a problem hiding this comment.
I think we talk at the daily to go through this. Its a bit difficult to understand all that is going on here. It seems actually like two PRs: (1) support constant divisors on DivRem; (2) combine q = x/y; r = x%y into one q,r = x%y to eliminate duplication. For (1), this actually seems fine to me (modulo the issue around supporting only a single limb); For (2), it seems like a lot of effort for something which is only used once in the arithmetisation (technically, its used twice --- but the second use can be rewritten). The simpler solution to support (2) is to introduce a dedicated expression form, such as x//y as I suggested in the comments. Anyway, this could still be merged as is, and then we put in the // expression separately later so we can remove planDivRemMerges().
| subZ1 = bytecode.SubConst(z1, []bytecode.RegisterId{rw1}, y.AsConstant()) | ||
| } else { | ||
| mulQY = bytecode.MulConst(qy, []bytecode.RegisterId{q, y.AsRegister()}, one) | ||
| subZ1 = bytecode.SubConst(z1, []bytecode.RegisterId{y.AsRegister(), rw1}, zero) |
There was a problem hiding this comment.
I think some kind of special instruction to manage this form at some point might be useful.
There was a problem hiding this comment.
I don't understand what you mean here ?
| return nvecs | ||
| } | ||
|
|
||
| // splitOperandVectors splits each operand (e.g. an intrinsic argument) into |
There was a problem hiding this comment.
This is so wierd. I guess this is something to do with the fact that a constant operand does have bitwidths for the limbs?? But, in fact, we know what their bitwidths are.
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
/% new operation
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 68 changed files in this pull request and generated no new comments.
Suppressed comments (5)
pkg/zkc/vm/internal/transform/lower_division.go:1
- qyr is sized to nX bits, but it is assigned qy + r. Even though mathematically q*y + r == x fits in nX bits, the constraint system needs to prevent the intermediate addition from wrapping; the prior approach of allocating qyr at nX+1 guarded the transient carry. Consider restoring qyr to nX+1 and ensuring the subsequent zero-assert compares the full (non-wrapping) sum against x (e.g., by extending x or otherwise constraining the carry limb).
pkg/zkc/vm/internal/interpreter/interpreter.go:1 - Fix grammar in panic message: "expect" → "expects".
testdata/zkc/unit/div_05.zkc:1 - The previous comment explained that this file is a regression test for division optimization across control-flow (divisor assigned different power-of-two constants on different paths). Removing that context makes the test harder to understand/maintain. Consider reintroducing a short comment describing the intent of this unit test.
pkg/zkc/vm/internal/bytecode/div.go:36 - DivRem.Divisor is documented as a single-limb operand, but Validate() only checks the constant is non-zero and does not enforce the single-limb invariant. Since encoding/execution paths for DIVC/REMC and intrinsic-constant operands assume a single value, DivRem.Validate should reject multi-limb constant divisors (e.g., len(p.Divisor.AsConstants()) != 1) to prevent producing bytecode that cannot be encoded/decoded correctly.
// Divisor is either a (single limb) operand register or a constant.
Divisor Operand[W]
pkg/zkc/vm/internal/bytecode/intrinsic.go:101
- This error message is misleading: len(s.AsConstants()) is the number of limbs in a single constant operand, not the number of constant operands. Consider changing the message to something like "constant operand has %d limb(s), expected 1" to more accurately reflect the validation being performed.
for _, s := range p.Sources {
if s.IsConstant() && len(s.AsConstants()) != 1 {
errs = append(errs, fmt.Errorf("%d constant operand(s) found, expected 1", len(s.AsConstants())))
}
}
No description provided.