-
Notifications
You must be signed in to change notification settings - Fork 128
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
keccak: Fix keccakf1600 implementation selection #168
Conversation
c0a686b
to
b1a9e28
Compare
1. On macOS __builtin_cpu_init() must be called directly because otherwise it will run after select_keccakf1600_implementation(). See https://bugs.llvm.org/show_bug.cgi?id=48459. 2. Check availability of both BMI and BMI2. Some CPUs like Intel E5-2697 v2 incorrectly report BMI2 being available. Co-authored-by: yperbasis <[email protected]>
b1a9e28
to
b816354
Compare
// This is needed on macOS because of the bug: https://bugs.llvm.org/show_bug.cgi?id=48459. | ||
__builtin_cpu_init(); | ||
|
||
// Check if both BMI and BMI2 are supported. Some CPUs like Intel E5-2697 v2 incorrectly |
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.
Do you have some kind of bug report / errata link for this?
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. This comes from experiments on CI.
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 can't even find any details spec of E5-2697 v2. I checks the assembly code generated by __builtin_cpu_supports("bmi2")
and it looks correct. I also found a case where for some other Intel CPU it was reporting some features not actually available. That was fixed by BIOS update :)
Seems like a CPU bug, but no more information than this.
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 also found a case where for some other Intel CPU it was reporting some features not actually available. That was fixed by BIOS update :)
That sounds like a new microcode was pushed to the BIOS. The microcode must be loaded on every cold start.
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 1651 1652 +1
=========================================
+ Hits 1651 1652 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
otherwise it will run after select_keccakf1600_implementation().
See https://bugs.llvm.org/show_bug.cgi?id=48459.
Intel E5-2697 v2 incorrectly report BMI2 being available.