Skip to content

fix: remove unused fields in rvstate#2887

Merged
shuklaayush merged 1 commit into
develop-v2.1.0-rv64from
feat/rvr-remove-dead-rvstate-fields
Jun 14, 2026
Merged

fix: remove unused fields in rvstate#2887
shuklaayush merged 1 commit into
develop-v2.1.0-rv64from
feat/rvr-remove-dead-rvstate-fields

Conversation

@mansur20478

Copy link
Copy Markdown

removes unused reservation_addr, reservation_valid, brk, start_brk, csrs fields from RvState.

@mansur20478 mansur20478 self-assigned this Jun 12, 2026
@github-actions

This comment has been minimized.

@mansur20478 mansur20478 marked this pull request as ready for review June 12, 2026 19:03
@mansur20478 mansur20478 requested a review from shuklaayush June 12, 2026 19:03
@shuklaayush

Copy link
Copy Markdown
Collaborator

@claude @codex there might've been a performance reason why we added this. do we not need it anymore?

    /// Alignment padding.
    _pad0: u8,

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @shuklaayush's task in 1m 48s —— View job


Investigating _pad0 alignment padding field

  • Read the current state of RvState and the PR diff
  • Investigate git history for why _pad0 was added
  • Analyze whether the padding is still needed for performance/alignment

Answer

The _pad0: u8 field is not needed anymore and is safe to remove. It was explicit alignment padding, not a performance optimization.

Why it existed (old layout):

In the old struct, the field sequence around _pad0 was:

reservation_addr: u32    // offset 280, size 4
reservation_valid: u8    // offset 284, size 1
has_exited: u8           // offset 285, size 1
exit_code: u8            // offset 286, size 1
_pad0: u8                // offset 287, size 1  ← explicit pad
brk: u32                 // offset 288, size 4  ← needs 4-byte alignment
start_brk: u32           // offset 292, size 4
memory: *mut u8          // offset 296, size 8

After three consecutive u8 fields (reservation_valid, has_exited, exit_code), we're at offset 287. The next field brk: u32 needs 4-byte alignment (offset 288). _pad0 was an explicit padding byte to keep the Rust repr(C) layout in exact sync with the handwritten C struct, ensuring no implicit padding surprises in the FFI boundary.

Why it's safe to remove (new layout):

With reservation_addr, reservation_valid, brk, start_brk, and csrs all gone, the layout becomes:

target_instret: u64      // offset 272, size 8
has_exited: u8           // offset 280, size 1
exit_code: u8            // offset 281, size 1
// (6 bytes implicit padding — repr(C) aligns to next field's requirement)
memory: *mut u8          // offset 288, size 8  ← needs 8-byte alignment

The compiler handles the padding between exit_code and memory automatically in both repr(C) Rust and C. Both the Rust struct and C struct agree on the layout without any explicit padding field.

tl;dr: _pad0 existed to align the now-removed brk: u32 field. No performance concern — just FFI layout matching.


@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 4813b4b58b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@shuklaayush shuklaayush left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm. let's merge this after the parent is merged

Base automatically changed from feat/rvr-support-rv64-riscv to develop-v2.1.0-rv64 June 14, 2026 08:37
@shuklaayush shuklaayush force-pushed the feat/rvr-remove-dead-rvstate-fields branch from 4813b4b to 0924674 Compare June 14, 2026 08:39
@shuklaayush shuklaayush merged commit 5378410 into develop-v2.1.0-rv64 Jun 14, 2026
18 checks passed
@shuklaayush shuklaayush deleted the feat/rvr-remove-dead-rvstate-fields branch June 14, 2026 08:39
@github-actions

Copy link
Copy Markdown
group app.proof_time_ms app.cycles leaf.proof_time_ms
fibonacci 1,661 4,000,051 527
keccak 16,369 14,365,133 3,039
sha2_bench 10,404 11,167,961 1,946
regex 1,543 4,090,656 432
ecrecover 482 112,210 312
pairing 627 592,827 304
kitchen_sink 3,934 1,979,971 867

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: 0924674

Benchmark Workflow

shuklaayush pushed a commit that referenced this pull request Jun 19, 2026
removes unused `reservation_addr`, `reservation_valid`, `brk`,
`start_brk`, `csrs` fields from RvState.
shuklaayush pushed a commit that referenced this pull request Jun 19, 2026
removes unused `reservation_addr`, `reservation_valid`, `brk`,
`start_brk`, `csrs` fields from RvState.
shuklaayush pushed a commit that referenced this pull request Jun 19, 2026
removes unused `reservation_addr`, `reservation_valid`, `brk`,
`start_brk`, `csrs` fields from RvState.
shuklaayush pushed a commit that referenced this pull request Jun 19, 2026
removes unused `reservation_addr`, `reservation_valid`, `brk`,
`start_brk`, `csrs` fields from RvState.
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