Skip to content

feat: rv64 loadstore#2714

Merged
shuklaayush merged 2 commits intorv64-mul-w-divrem-wfrom
rv64-loadstore
Apr 17, 2026
Merged

feat: rv64 loadstore#2714
shuklaayush merged 2 commits intorv64-mul-w-divrem-wfrom
rv64-loadstore

Conversation

@876pol
Copy link
Copy Markdown
Contributor

@876pol 876pol commented Apr 17, 2026

Resolves INT-7500.

Summary:

  • loadstore submodule of riscv has been ported from RV32 to RV64
    • All types/structs renamed from Rv32* to Rv64* (e.g., Rv32LoadStoreAdapterAirRv64LoadStoreAdapterAir)
    • Operand width changed from u32/4 bytes to u64/8 bytes throughout (RV32_REGISTER_NUM_LIMBSRV64_REGISTER_NUM_LIMBS, RV32_*_ASRV64_*_AS, rs1_val: u32u64) + loadstore now handles the LOADD, LOADWU, and STORED opcodes
  • Rv64LoadStoreAdapter updated for RV64
    • New constraint added: upper 4 bytes of rs1_data must be zero, since this adapter only supports 32-bit memory addresses
  • LoadStoreCoreAir rewritten to use the Encoder primitive in place of the ad-hoc flags: [T; 4] scheme, since the 30 valid (opcode, shift) pairs no longer fit in the old encoding
    • New InstructionCase enum enumerates all 30 cases; selector: [T; 7] replaces flags: [T; 4]

@876pol 876pol marked this pull request as ready for review April 17, 2026 18:47
@shuklaayush
Copy link
Copy Markdown
Collaborator

@claude can you review the loadstore chip changes, specifically whether the encoder subair is used correctly

@claude
Copy link
Copy Markdown

claude bot commented Apr 17, 2026

Claude finished @shuklaayush's task in 3m 52s —— View job


Reviewing loadstore chip changes (encoder subair focus)

  • Read repo guidelines (CLAUDE.md)
  • Read the Encoder primitive implementation
  • Review loadstore/core.rs changes (encoder usage)
  • Review adapters/loadstore.rs changes
  • Review loadstore/execution.rs changes
  • Review loadstore/tests.rs changes
  • Post review feedback

Review: Encoder SubAir Usage in LoadStore Chip

Overall: The encoder subair is used correctly. The integration is sound and the constraint logic is well-structured. Here are the details:

Encoder Construction ✅

// core.rs:169-177
fn loadstore_encoder() -> Encoder {
    Encoder::new(LOADSTORE_SELECTOR_CASES, LOADSTORE_SELECTOR_MAX_DEGREE, true)
    // 30 flags, max_degree=2, reserve_invalid=true
}
  • 30 flags + 1 reserved invalid point = 31 used points
  • With k=7 variables and max_degree=2: C(9,7) = 36 total points ≥ 31 ✅
  • With k=6: C(8,6) = 28 < 31, so k=7 is the minimum ✅
  • reserve_invalid=true correctly reserves (0,...,0) for padding rows, so uninitialized (zeroed) rows are treated as invalid ✅

Constraint Application ✅

// core.rs:255
self.encoder.eval(builder, &selector);

This applies 3 constraint families from the encoder:

  1. Each variable is in {0, 1, 2} (degree 3)
  2. Sum of variables is in {0, 1, 2} (degree 3)
  3. sum_of_unused = 0 — forces the point to be one of the 31 valid encodings

Flag Extraction & Derived Columns ✅

// core.rs:257-272
let selector_flags = self.encoder.flags::<AB>(&selector); // 30 expressions, each degree 2
let expected_is_valid = selector_flags.iter().fold(...);   // sum of flags = 0 or 1
let expected_is_load = /* sum of load-case flags */;
builder.assert_eq(is_valid, expected_is_valid);
builder.assert_eq(is_load, expected_is_load);
  • is_valid and is_load are stored as separate degree-1 columns, correctly avoiding degree blowup in the adapter. The adapter multiplies is_valid by other degree-1 terms, so keeping it at degree 1 is essential.
  • Summing all 30 flag expressions is equivalent to encoder.is_valid() since eval forces sum_of_unused = 0. ✅

Shift Amount Splitting ✅

// core.rs:282-301
let load_shift_amount = /* sum of flag * shift for load cases */;
let store_shift_amount = /* sum of flag * shift for store cases */;

These are degree 2 (flag expr × constant). Keeping them separate avoids the degree-3 blowup that would occur if a single shift_amount were used inside select(is_load, ...) in the adapter. The adapter comment at loadstore.rs:235-238 confirms this design rationale.

write_data Constraints ✅

// core.rs:303-324
for (i, cell) in write_data.iter().enumerate() {
    let expected = InstructionCase::ALL.iter().fold(..., |acc, &case| {
        acc + selector_flags[case as usize].clone() * term  // degree 2 × degree 1 = degree 3
    });
    builder.assert_eq(*cell, expected);
}

Max constraint degree = 3, matching the encoder's own constraint degree. ✅

Trace Filling ✅

// core.rs:454-455
core_row.selector = selector_point_for_opcode_shift(opcode, shift).map(F::from_canonical_u32);

Uses encoder.get_flag_pt(InstructionCase::from_opcode_shift(opcode, shift) as usize). The as usize cast on the enum discriminant correctly maps to the flag index since InstructionCase variants are declared in the same order as InstructionCase::ALL. ✅

InstructionCase Enum ✅

The enum covers exactly the 30 valid (opcode, shift) pairs for RV64 unsigned loads and stores. The from_opcode_shift function is exhaustive and unreachable!s on invalid combinations.


Minor Observations

  1. selector_point_for_opcode_shift creates a fresh Encoder on every call (core.rs:183). This function is called once per row during trace filling. The encoder construction involves generating all 36 points — not expensive but could be avoided with a LazyLock/OnceLock. Not a correctness issue.

  2. CUDA tests still reference Rv32 types (tests.rs:29-36, 782-861). The #[cfg(feature = "cuda")] blocks import Rv32LoadStoreAdapterRecord, Rv32LoadStoreChipGpu, RV32_REGISTER_NUM_LIMBS, etc. These would fail to compile with the cuda feature. Presumably intentional since the CUDA path needs a separate port, but worth confirming.

  3. Style nit: expected_is_valid could use self.encoder.is_valid::<AB>(&selector) instead of manually summing all flags. They're mathematically equivalent (given eval forces sum_of_unused = 0), but the encoder API method is more self-documenting.


Copy link
Copy Markdown
Collaborator

@shuklaayush shuklaayush left a comment

Choose a reason for hiding this comment

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

couple of nits. i didn't thoroughly review the constraints. we can do that later

Comment thread extensions/riscv/circuit/src/loadstore/core.rs
Comment thread extensions/riscv/circuit/src/loadstore/core.rs Outdated
@shuklaayush
Copy link
Copy Markdown
Collaborator

@claude can you apply the suggestions

@claude
Copy link
Copy Markdown

claude bot commented Apr 17, 2026

Claude finished @shuklaayush's task in 4m 38s —— View job


Applying review suggestions

  • Read current loadstore/core.rs to understand the code
  • Make encoder part of the filler struct (instead of recreating per call)
  • Use encoder.is_valid() instead of manually summing flags
  • Commit and push changes

Applied both review suggestions in core.rs:

  1. Encoder in filler struct: Added encoder: Encoder field to LoadStoreFiller with a custom new constructor. fill_trace_row now uses self.encoder.get_flag_pt(...) instead of calling selector_point_for_opcode_shift (which recreated the encoder on every row).

  2. encoder.is_valid(): Replaced the manual selector_flags.iter().fold(...) sum with self.encoder.is_valid::<AB>(&selector). Degree stays at 2 since the invalid point is (0,...,0).

- Add `encoder: Encoder` field to `LoadStoreFiller` to avoid recreating
  the encoder on every `fill_trace_row` call
- Use `self.encoder.is_valid()` instead of manually summing all selector
  flags to compute `expected_is_valid`

Co-authored-by: Ayush Shukla <shuklaayush@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@shuklaayush shuklaayush merged commit c23d708 into rv64-mul-w-divrem-w Apr 17, 2026
4 of 59 checks passed
@shuklaayush shuklaayush deleted the rv64-loadstore branch April 17, 2026 20:47
shuklaayush added a commit that referenced this pull request Apr 17, 2026
Resolves INT-7500.

Summary:
- `loadstore` submodule of `riscv` has been ported from RV32 to RV64
- All types/structs renamed from `Rv32*` to `Rv64*` (e.g.,
`Rv32LoadStoreAdapterAir` → `Rv64LoadStoreAdapterAir`)
- Operand width changed from `u32`/4 bytes to `u64`/8 bytes throughout
(`RV32_REGISTER_NUM_LIMBS` → `RV64_REGISTER_NUM_LIMBS`, `RV32_*_AS` →
`RV64_*_AS`, `rs1_val: u32` → `u64`) + loadstore now handles the
`LOADD`, `LOADWU`, and `STORED` opcodes
- `Rv64LoadStoreAdapter` updated for RV64
- New constraint added: upper 4 bytes of `rs1_data` must be zero, since
this adapter only supports 32-bit memory addresses
- `LoadStoreCoreAir` rewritten to use the `Encoder` primitive in place
of the ad-hoc `flags: [T; 4]` scheme, since the 30 valid `(opcode,
shift)` pairs no longer fit in the old encoding
- New `InstructionCase` enum enumerates all 30 cases; `selector: [T; 7]`
replaces `flags: [T; 4]`

---------

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Ayush Shukla <shuklaayush@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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.

2 participants