perf: inline constant shifts and lower rotation idioms - #2200
Open
gbotrel wants to merge 1 commit into
Open
Conversation
A shift by a compile-time constant amount is now realised inline as a destruct/concat of the source register, instead of a call into a synthesized barrel-shifter chain. An OR of two opposite shifts of the same source whose amounts sum to the operand width is recognised as a rotation: constant amounts are inlined the same way, dynamic ones call a dedicated rotation chain, cheaper than two shift chains plus an OR lookup. Concat splitting learns to borrow low bits from the next source limb when a target limb is wider than its chunk, which the odd-width limbs of inlined rotations require. keccak bench (4,236-byte message): 35.9M -> 29.3M trace cells (-18%). blake bench: 3.9M -> 3.2M (-19%). Signed-off-by: Gautam Botrel <gautam.botrel@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves ZkC VM bitwise lowering performance by (1) inlining constant shifts, (2) recognizing rotation idioms and lowering them to cheaper sequences (inline for constants; helper-chain for dynamic), and (3) enhancing concat splitting to support odd-width limb patterns produced by rotations.
Changes:
- Inline SHL/SHR when the shift amount is a compile-time constant (no barrel-shifter helpers / lookups).
- Add rotation recognition + lowering (constant rotations inline; dynamic rotations via a new rotation chain).
- Extend concat chunk formation to “borrow” low bits from subsequent source limbs when a target limb is wider than the current source chunk; add an end-to-end rotation unit fixture.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testdata/zkc/unit/rotate_01.zkc | New unit fixture exercising variable + constant rotation idioms. |
| testdata/zkc/unit/rotate_01.rejects | Reject traces for rotate_01 fixture. |
| testdata/zkc/unit/rotate_01.accepts | Accept traces for rotate_01 fixture. |
| pkg/zkc/vm/internal/transform/split/concat.go | Borrow bits from following limbs to avoid mid-stream zero-padding when targets are wider than the selected source chunk. |
| pkg/zkc/vm/internal/transform/rotation_scan.go | New rotation-idiom scanner used to rewrite OR-of-opposite-shifts into rotations. |
| pkg/zkc/vm/internal/transform/rot_helpers.go | New rotation helper-chain builder (rotl/rotr) for dynamic amounts. |
| pkg/zkc/vm/internal/transform/lower_shift.go | Extend shift helper key to distinguish rotation helpers from shift helpers. |
| pkg/zkc/vm/internal/transform/lower_bitwise.go | Wire in constant scanning + rotation detection; inline constant shifts; rewrite recognized rotations. |
| pkg/zkc/vm/internal/transform/constant_scan.go | New constant register scanner to enable constant shift inlining. |
| pkg/test/zkc_unit_test.go | Register the new rotate_01 unit test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+210
to
+212
| if sub.Constant.Cmp64(0) != 0 || sub.Source[1] != n { | ||
| return false | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
edit: not urgent to merge, just keeping around
Found while profiling the keccak bench.
What
(x << n) | (x >> (w - n))is now recognised as a rotation. Constant amounts are inlined the same way; dynamic amounts call a new rotation chain, which is cheaper than two shift chains plus an OR lookup.u49::u15used to panic).Numbers
zkc trace --stats, KOALABEAR_16:The new
rotate_01fixture alone drops from 11 helper modules (9 shift chain levels + 2 OR buses) to 4 rotation chain levels.