-
Notifications
You must be signed in to change notification settings - Fork 276
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
Implemented rdrand and rdseed intrinsics #326
Conversation
coresimd/x86/i686/rdrand.rs
Outdated
fn x86_rdseed16_step() -> (u16, i32); | ||
fn x86_rdseed32_step() -> (u32, i32); | ||
fn x86_rdseed64_step() -> (u64, i32); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use platform-intrinsic
in this crate. Can you link to the LLVM intrinsics directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, as was discussed in the issue LLVM type {I<width>, i32} can not be currently represented by Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that means we want to start using platform-intrinsics here.
We should probably wait for @alexcrichton to review this and help decide how to proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He suggested to use asm!
block as a temporary solution, so I think platform-intrinsics will be an ok temporary solution as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah the platform intrinsic ABI here is the way to go for now. This may change in the future, but we can always just update the crate!
In any case this PR looks good to me, just waiting on the rust-lang/rust PR!
@newpavlov there's a failure on CI related to the verification of intrinsics, but I think it's benign and mostly just means there's a missing case here. Mind fixing that in this PR as well? |
@alexcrichton |
Ok! I reran the builds with the new nightly, but it looks like there may be some errors? |
Not sure how to debug this error:
Maybe it's a bug in the current nigthly? Can you please try to restart test for |
Looks like rdrand64 is only available on x86_64 targets |
Oh I think it's just rdrand64, the other functions should be available on x86 |
Thanks! |
Fixes: #325
Depends on rust-lang/rust#48369.