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(evm_arithmetization): Adjust layout of CpuGeneralColumnsView #355

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

gio256
Copy link
Contributor

@gio256 gio256 commented Jul 2, 2024

Addresses #340.

I'm very open to suggestions if there is a way to make this less cumbersome.

It could be nice to add some miri tests for this and other conversions, but I can't think of a clean way to run them in the CI. Miri doesn't support inline assembly, so we would need a way to avoid running any test that touches assembly in plonky2. Adding cfg attributes to every other test seems prohibitive. Maybe a naming scheme for miri tests and a cargo test filter would be good enough?

@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Jul 2, 2024
@gio256 gio256 changed the title fix(evm_arithmetization): Adjust layout of CpuGeneralColumnsView fix(evm_arithmetization): Adjust layout of CpuGeneralColumnsView Jul 4, 2024
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

LGTM, minus a tiny nit and a potential request (upon feasibility).

Comment on lines 168 to 171
/// Reserve the unused columns at the beginning. This allows `Self` to
/// coexist with any view that uses only the first four columns (i.e. all
/// except `CpuLogicView`).
_unused: [T; NUM_UNION_COLUMNS - 4],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, we'd also have some const_assert on all these above that ensures that the "unpadded" length of these variants (apart from LogicView) is actually smaller than the _unused field of Cpu (to ensure coexistence viability). But I'm not sure this is possible with current Rust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't immediately see a way to make an assertion about the unpadded size of a field without generic_const_exprs. Maybe that's an argument for not padding in the first place?

With the padding, the two options I see are:

  • The small refactor mentioned above (splitting into two unions of different sizes).
  • Adding some manual assertions more-or-less as "documentation". e.g.
/// View of the first three `CpuGeneralColumns` containing exception code bits.
#[repr(C)]
#[derive(Copy, Clone)]
pub(crate) struct CpuExceptionView<T: Copy> {
    /// Exception code as little-endian bits.
    pub(crate) exc_code_bits: [T; 3],
    /// Reserve the unused columns.
    _padding_columns: [T; NUM_UNION_COLUMNS - 3],
}
const_assert!(3 <= NUM_COLUMNS_SHARED_WITH_STACK);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise we can have it as unit test, so that we can instantiate it concretely and then do the bound checks on the fields themselves. But non-blocking

evm_arithmetization/src/cpu/columns/general.rs Outdated Show resolved Hide resolved
@Nashtare Nashtare merged commit d8fbb77 into 0xPolygonZero:develop Jul 9, 2024
14 checks passed
@Nashtare Nashtare added this to the Type 1 - Q3 2024 milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants