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

More SIMD opcodes #732

Merged
merged 1 commit into from
Jan 9, 2025
Merged

More SIMD opcodes #732

merged 1 commit into from
Jan 9, 2025

Conversation

andreaTP
Copy link
Collaborator

@andreaTP andreaTP commented Jan 9, 2025

cc. @mfirry

@andreaTP andreaTP requested review from electrum and evacchi January 9, 2025 12:08
@mfirry
Copy link
Contributor

mfirry commented Jan 9, 2025

LGTM! My review can mainly focus on the SimdInterpreterMachine class.
One thought I had: as this file continues to grow (likely reaching 1000+ lines soon), should we consider reorganizing it into smaller modules or splitting the logic across multiple files to improve maintainability?

@andreaTP
Copy link
Collaborator Author

andreaTP commented Jan 9, 2025

should we consider reorganizing it into smaller modules or splitting the logic across multiple files

In general, I think it's expected that the OpCodes implementation will take some lines of code ...
for example, the current InterpreterMachine is > 2000 LOCs but is still manageable and organized well enough.
Splitting in multiple files not always improves readability and maintainability.

That said, @mfirry , if you have a proposal, go ahead and we can evaluate it, alternatively we will face the problem when it hurts.

Copy link
Collaborator

@evacchi evacchi left a comment

Choose a reason for hiding this comment

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

looks good, as long as the tests are passing 😎

@andreaTP andreaTP merged commit 096cc59 into dylibso:main Jan 9, 2025
16 checks passed
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.

3 participants