perf: enable partial function call and static memory read - #2191
Conversation
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 adds first-class support for partial bindings in ZkC (using _ to discard selected return values) across the compiler, VM bytecode/encoding, and constraint emission, and includes new fixtures/tests to validate the behavior for both function calls and static-memory reads.
Changes:
- Introduces a discardable assignment target (
_) via a newlval.DiscardAST node and threads it through parsing, typing, linking, lowering, and codegen using aDISCARDpseudo-register. - Updates VM transforms, interpreter encoding, gogen, and constraint lookup emission to correctly handle discarded call returns and discarded static-memory data columns.
- Adds unit/invalid fixtures and Go tests covering partial-call patterns and rejecting illegal
_usage.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/zkc/unit/partial_call_01.zkc | Unit fixture: discard second return in a 2-tuple function call. |
| testdata/zkc/unit/partial_call_01.accepts | Accepted traces for partial_call_01. |
| testdata/zkc/unit/partial_call_02.zkc | Unit fixture: discard first return in a 2-tuple function call. |
| testdata/zkc/unit/partial_call_02.accepts | Accepted traces for partial_call_02. |
| testdata/zkc/unit/partial_call_03.zkc | Unit fixture: discard first/third returns in a 3-tuple function call. |
| testdata/zkc/unit/partial_call_03.accepts | Accepted traces for partial_call_03. |
| testdata/zkc/unit/partial_call_04.zkc | Unit fixture: discard middle return in a 3-tuple function call. |
| testdata/zkc/unit/partial_call_04.accepts | Accepted traces for partial_call_04. |
| testdata/zkc/unit/partial_call_05.zkc | Unit fixture: discard one column from a multi-column static memory read. |
| testdata/zkc/unit/partial_call_05.accepts | Accepted traces for partial_call_05. |
| testdata/zkc/invalid/partial_call_01.zkc | Invalid fixture: rejects discarding all returns in an assignment. |
| testdata/zkc/invalid/partial_call_02.zkc | Invalid fixture: mismatched return arity (too few targets). |
| testdata/zkc/invalid/partial_call_03.zkc | Invalid fixture: mismatched return arity (too many targets). |
| testdata/zkc/invalid/basic_42.zkc | Invalid fixture: rejects declaring _ as a variable. |
| testdata/zkc/invalid/basic_43.zkc | Invalid fixture: rejects calling a function named _. |
| testdata/zkc/invalid/basic_44.zkc | Invalid fixture: rejects reading from _ as an identifier. |
| testdata/zkc/invalid/basic_45.zkc | Invalid fixture: rejects declaring a function named _. |
| pkg/zkc/vm/internal/transform/split_registers.go | Keeps call boundaries aligned when returns are discarded during register splitting. |
| pkg/zkc/vm/internal/transform/lower_shift.go | Refactors shift helper construction (builder) to better control registers/temps. |
| pkg/zkc/vm/internal/transform/lower_or_xor_and.go | Merges AND/OR/XOR helpers into a single multi-output helper; supports discarding unused outputs. |
| pkg/zkc/vm/internal/transform/inline_functions.go | Ensures inlining/shadow-mapping logic treats discarded returns as unbound. |
| pkg/zkc/vm/internal/transform/check_casts.go | Skips cast checks for discarded return bindings. |
| pkg/zkc/vm/internal/interpreter/encoding/readwrite.go | Densifies static-read data bindings so positional executors bind remaining lines correctly. |
| pkg/zkc/vm/internal/interpreter/encoding/call.go | Densifies call return bindings for LEAVE encoding; adds denseBindings helper. |
| pkg/zkc/vm/internal/gogen/emit_memory.go | Skips loads for discarded static memory data lines. |
| pkg/zkc/vm/internal/gogen/emit_call.go | Emits blanks in Go tuple assignments for discarded returns. |
| pkg/zkc/vm/internal/bytecode/util.go | Renders DISCARD as _ and adds boundRegisters helper to filter discards. |
| pkg/zkc/vm/internal/bytecode/readwrite.go | Excludes discarded read data from Definitions; validates discards only allowed on read data. |
| pkg/zkc/vm/internal/bytecode/call.go | Excludes discarded returns from Definitions; validates discards only allowed on returns. |
| pkg/zkc/vm/internal/bytecode/bytecode.go | Introduces DISCARD pseudo register id and updates operand validation accordingly. |
| pkg/zkc/vm/bytecode.go | Re-exports DISCARD for external VM consumers. |
| pkg/zkc/constraints/translator.go | Computes padded static height from row count (StaticHeight) rather than flattened contents length. |
| pkg/zkc/constraints/call_and_memory_lookup.go | Omits discarded (source,target) pairs from call/memory lookup constraints. |
| pkg/zkc/compiler/validate/typing.go | Adds dedicated typing path for assignments with discarded targets (_). |
| pkg/zkc/compiler/parser/parser.go | Reserves _ for discard targets; rejects it for declarations and reads. |
| pkg/zkc/compiler/lower/flatten_fixed_array.go | Preserves discard lvals during fixed-array lowering. |
| pkg/zkc/compiler/linker.go | Links discard lvals into resolved AST. |
| pkg/zkc/compiler/codegen/statement.go | Maps discard lvals to the DISCARD pseudo-register; clarifies destructuring behavior. |
| pkg/zkc/compiler/codegen/compile.go | Flattens tuple static initializers row-major to support multi-column static memories. |
| pkg/zkc/compiler/ast/lval/lval.go | Adds stringification support for discard lvals. |
| pkg/zkc/compiler/ast/lval/discard.go | Introduces the discard lval node (lval.Discard). |
| pkg/test/zkc_unit_test.go | Registers new unit tests for partial-call/static-read discard behavior. |
| pkg/test/zkc_invalid_test.go | Registers new invalid tests for _-related parse/typing failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // This test perform a partial call res, _ = f(x) | ||
| func Test_ZkcUnit_partial_call_01(t *testing.T) { | ||
| checkZkcUnit(t, "zkc/unit/partial_call_01", DEFAULT_UNIT_CONFIG) | ||
| } |
| // =================================================================== | ||
| // Unreachable module from main | ||
| // =================================================================== | ||
| func Test_ZkcInvalid_Partial_call_01(t *testing.T) { | ||
| checkZkcInvalid(t, "zkc/invalid/partial_call_01") | ||
| } |
| mask = (uint64(1) << width) - 1 | ||
| rows = uint64(1) << (2 * width) | ||
| contents = make([]W, rows) | ||
| contents = make([]W, 0, rows*uint64(len(bitwiseOps))) |
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
pkg/test/zkc_unit_test.go:1451
- Grammar: “This test performs …” (subject–verb agreement).
// This test perform a partial call res, _ = f(x)
pkg/test/zkc_unit_test.go:1456
- Grammar: “This test performs …” (subject–verb agreement).
// This test perform a partial call _, res = f(x)
pkg/test/zkc_unit_test.go:1461
- Grammar: “This test performs …” (subject–verb agreement).
// This test perform a partial call _, res, _ = f(x)
pkg/test/zkc_unit_test.go:1466
- Grammar: “This test performs …” (subject–verb agreement).
// This test perform a partial call res, _, res = f(x)
pkg/test/zkc_unit_test.go:1471
- Grammar: “This test performs …” (subject–verb agreement).
// This test perform a partial static call res, _ = static(x)
DavePearce
left a comment
There was a problem hiding this comment.
Overall, I think this looks pretty good. I have requested some changes, but they are mostly pretty minor.
| } | ||
| // Sanity check at least one return value is bound | ||
| if !array.ContainsMatching(s.Targets, func(lv LVal) bool { return !isDiscard(lv) }) { | ||
| return p.srcmaps.SyntaxErrors(s, "at least one return argument must be used") |
There was a problem hiding this comment.
Why enforce this requirement? Its not necessary.
There was a problem hiding this comment.
It's not necessary, but I see it as a "garde-fou". I don't see any use case where we would like _ = f(x)
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
DavePearce
left a comment
There was a problem hiding this comment.
Overall, looks good. The change merging typePartialAssignment() into typeAssignment was not done well, but otherwise looks good.
| // Partial assignment: one or more targets are the wildcard "_". | ||
| // Discarding is only meaningful for sources whose return types are fixed by | ||
| // a declaration (function calls and static memory reads). | ||
| if array.ContainsMatching(s.Targets, isDiscard) { |
There was a problem hiding this comment.
Oh, I love this so much. I can only hope that Claude is solely responsible ;)
No description provided.