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.

Sign up for GitHub

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

Jump to bottom

refactored from() method for MemoryOp array impl #591

Merged
merged 6 commits into from
Feb 20, 2025
Merged

refactored from() method for MemoryOp array impl #591

merged 6 commits into from
Feb 20, 2025

Conversation

Roee-87
Copy link
Contributor

@Roee-87 Roee-87 commented Feb 16, 2025

This PR refactors the from(val: &RVTraceRow) method. Instead of matching an instruction type to an array of memory operations, the method now matches all of the RV32IM instructions to a specific memory operation, including the RV32IM::ECALL instruction.

@Roee-87 Roee-87 marked this pull request as ready for review February 16, 2025 00:28
@Roee-87
Copy link
Contributor Author

Roee-87 commented Feb 16, 2025

Note: The following RV32IM ops are not matched to any memory operation since the original code did not match them:
LB, LH, LBU, and LHU .
I'm unsure if I should leave them unimplemented, or have them match to the same memory operations as LW.

moodlezoup
moodlezoup reviewed Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

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

thanks! Will merge as soon as the fmt and clippy errors are fixed

common/src/rv_trace.rs
Comment on lines +199 to +204
RV32IM::JAL => [
MemoryOp::noop_read(),
MemoryOp::noop_read(),
rd_write(),
MemoryOp::noop_read(),
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

JAL appears twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the duplicate: b7e93f9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran cargo fmt: 7388138

@moodlezoup moodlezoup merged commit c93148d into a16z:main Feb 20, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moodlezoup moodlezoup moodlezoup left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Roee-87 @moodlezoup