Skip to content

Faster PoT verification for CPUs that support AVX512F+VAES #3552

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

Merged
merged 4 commits into from
Jun 2, 2025

Conversation

nazar-pc
Copy link
Member

Backport of nazar-pc/abundance#260, doubles PoT verification performance on my Zen 4 machine on top of #3551, which itself is ~10% faster than main.

Would be nice for someone to invest time into optimizing verification for Aarch64, there are both generic and low-level optimized examples now.

To be merged after #3551.

Code contributor checklist:

@nazar-pc nazar-pc force-pushed the pot-avx512f+vaes branch from 4bd0793 to 4508aee Compare May 28, 2025 07:02
@nazar-pc
Copy link
Member Author

Looks like Rust toolchain upgrade is needed here, will wait for #3535

Base automatically changed from faster-pot-verification to main May 28, 2025 13:00
teor2345
teor2345 previously approved these changes May 28, 2025
Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks fine to me once compilation is fixed.

@teor2345
Copy link
Member

Looks like Rust toolchain upgrade is needed here, will wait for #3535

From the tracking issue it looks like there's a CPU feature missing, and it's still unstable in nightly:
rust-lang/rust#44839

@@ -1,6 +1,6 @@
//! Proof of time implementation.

#![cfg_attr(target_arch = "x86_64", feature(stdarch_x86_avx512))]
#![cfg_attr(target_arch = "x86_64", feature(avx512_target_feature, stdarch_x86_avx512))]
Copy link
Member Author

@nazar-pc nazar-pc May 28, 2025

Choose a reason for hiding this comment

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

There are more problems with code than this. And this will not be needed once Rust toolchain is upgraded: rust-lang/rust#138940

In fact soon stdarch_x86_avx512 will not be needed either: rust-lang/rust#111137 (comment)

@nazar-pc
Copy link
Member Author

Last commit is neither necessary nor sufficient, just wait for toolchain to be upgraded and it will all light up green and compile successfully.

@teor2345
Copy link
Member

teor2345 commented May 28, 2025

Last commit is neither necessary nor sufficient, just wait for toolchain to be upgraded and it will all light up green and compile successfully.

We'll need a later nightly, the current one in PR #3535 does not include stabilisation of avx512
Edit: #3535 (comment)

@nazar-pc
Copy link
Member Author

True, but it is fairly trivial to upgrade there from where #3535 stands

@teor2345
Copy link
Member

True, but it is fairly trivial to upgrade there from where #3535 stands

I've asked for the later nightly, we might as well do it all at once.

@nazar-pc nazar-pc force-pushed the pot-avx512f+vaes branch from 3561e14 to bb4d9af Compare May 29, 2025 03:57
@nazar-pc
Copy link
Member Author

Pushed another commit that reduces usage of unsafe to the very minimum required by the fact that platform-specific features are used in certain functions. VAES intrinsics are unsafe for unknown reason, so I filed an upstream issue for now.

This should make life easier for auditors.

@nazar-pc
Copy link
Member Author

Backported nazar-pc/abundance#269 too in the last commit, it implements VAES support without AVX512F and even regular AES-NI support for older CPUs, basically covering everything on x86-64 side.

@nazar-pc
Copy link
Member Author

And I ended up implementing optimized version of both proving and verification for aarch64 too (no SVE AES because no intrinsics in standard library yet): nazar-pc/abundance#270

Will submit as a separate PR once this is merged.

@teor2345
Copy link
Member

teor2345 commented Jun 2, 2025

VAES intrinsics are unsafe for unknown reason, so I filed an upstream issue for now.

These have now been marked as safe:
rust-lang/stdarch#1810

@nazar-pc
Copy link
Member Author

nazar-pc commented Jun 2, 2025

Yes, but stdarch git module has not yet been updated in the compiler repository: https://github.com/rust-lang/rust/tree/master/library
Last update was 2 weeks ago.

@teor2345
Copy link
Member

teor2345 commented Jun 2, 2025

Yes, but stdarch git module has not yet been updated in the compiler repository: https://github.com/rust-lang/rust/tree/master/library Last update was 2 weeks ago.

I understand it will take some time to upstream, this was more about any reviews/audits of the "unsafe" code.

@nazar-pc
Copy link
Member Author

nazar-pc commented Jun 2, 2025

That unsafe isn't really critical, I just find it quite pleasant that with experimental SIMD and recent compiler version it is possible to write mostly safe code even when using low-level intrinsics, it is pretty cool.

@teor2345
Copy link
Member

teor2345 commented Jun 2, 2025

This is now ready for a merge with the 2025-05-31 nightly update.

# Conflicts:
#	crates/subspace-proof-of-time/src/aes/x86_64.rs
@nazar-pc nazar-pc enabled auto-merge June 2, 2025 08:48
@nazar-pc nazar-pc requested a review from teor2345 June 2, 2025 08:48
Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks!

@nazar-pc nazar-pc added this pull request to the merge queue Jun 2, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 2, 2025
@nazar-pc nazar-pc merged commit adadb17 into main Jun 2, 2025
12 checks passed
@nazar-pc nazar-pc deleted the pot-avx512f+vaes branch June 2, 2025 11:15
@nazar-pc
Copy link
Member Author

nazar-pc commented Jun 8, 2025

AVX512 intrinsics were just stabilized in nightly-2025-06-08 and unsafe is no longer needed for them as well.

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