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

feat: implement RPO hash using SVE instructions #190

Merged
merged 2 commits into from Sep 25, 2023

Conversation

gswirski
Copy link
Contributor

@gswirski gswirski commented Sep 20, 2023

This PR addresses #158. It improves performance of the RPO hash function by leveraging SVE instructions on compatible ARMv8+SVE hardware. On the current generation of Amazon Graviton3 processors, I'm measuring 37% improvement against the baseline commit 90dd3ac (AWS c7g.medium instance).

To leverage SVE implementation, code needs to be compiled with --features arch-arm64-sve. Due to high latency of vector operations, only 2/3 of array elements are processed using SIMD. The rest can be processed for "free" using scalar instructions and masking the latency.

Below cargo bench --features arch-arm64-sve improvement against the baseline cargo bench.

Screenshot 2023-09-20 at 11 57 25

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@gswirski gswirski changed the title feat: implement RPO hash using SVE instructionss feat: implement RPO hash using SVE instructions Sep 20, 2023
@bobbinth
Copy link
Contributor

Awesome work! Thank you! I'll do a full review over the next couple of days - but I did ran the benchmarks on Graviton 3 and can confirm that I'm getting the same improvement. Very cool!

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you again! I left a few small nits inline and also one comment about adding a couple of Rust-based tests.

One other thing I'm thinking about is how to fix the CI (we run tests with all-features enabled). As far as I can tell, Graviton3 does not expose a feature flag to identify the SVE feature. One way to get around this is to assume that if the architecture is arm64 and OS is linux, then we are on Graviton3 - but it feels a bit hacky. Another way is to modify how we run CI tests - but I'm not sure how yet.

Any thoughts on this?

arch/arm64-sve/library.c Outdated Show resolved Hide resolved
src/hash/rpo/mod.rs Show resolved Hide resolved
src/hash/rpo/mod.rs Show resolved Hide resolved
arch/arm64-sve/test.c Outdated Show resolved Hide resolved
@gswirski
Copy link
Contributor Author

Another way is to modify how we run CI tests - but I'm not sure how yet.

I would suggest replacing --all-features with manually listing all features except for arch-arm64-sve.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you!

@bobbinth bobbinth merged commit 3964186 into 0xPolygonMiden:next Sep 25, 2023
9 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.

None yet

2 participants