Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: alignment in unsafe rust #1486

Merged
merged 4 commits into from
Mar 20, 2025
Merged

fix: alignment in unsafe rust #1486

merged 4 commits into from
Mar 20, 2025

Conversation

jonathanpwang
Copy link
Contributor

@jonathanpwang jonathanpwang commented Mar 19, 2025

Rust treats [u8; LEN] as having alignment of 1, so if you unsafely transmute or convert a slice to [T; LEN] which expects alignment of align_of<T>(), then this may result in misaligned load/store instructions, often during operations like memcpy. OpenVM RV32 does not support misaligned access (see #1474), so we make sure all our use of unsafe Rust has proper alignment.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@jonathanpwang jonathanpwang marked this pull request as ready for review March 19, 2025 21:42

This comment has been minimized.

Copy link

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (+14 [+1.1%]) 1,234 333,944 17,901,400 - - -
fibonacci_program (-7 [-0.3%]) 2,683 1,500,096 51,485,167 - - -
regex_program (+46 [+0.6%]) 7,759 4,140,240 167,400,358 - - -
ecrecover_program 1,417 295,291 15,598,160 - - -
pairing (-40 [-0.8%]) 4,739 1,711,640 92,620,923 - - -

Commit: 92d0bba

Benchmark Workflow

} else {
panic!("limbs must be at most 48");
}

let block_size = proc_macro::Literal::usize_unsuffixed(block_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow good to know this exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait, I copied this from code you wrote!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I thought I used some hacks for this, but maybe this was the hack...

Copy link
Contributor

@Golovanov399 Golovanov399 left a comment

Choose a reason for hiding this comment

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

Hope it's all and hope no major performance issues are introduced

@jonathanpwang jonathanpwang merged commit fe9b582 into main Mar 20, 2025
17 checks passed
@jonathanpwang jonathanpwang deleted the fix/unsafe-alignment branch March 20, 2025 00:57
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