Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-130213: Check availability of Intel SIMD types#130332
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
jmroot commented Feb 20, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
msprotz left a comment
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.
This is as discussed and matches my understanding of the problem. Thank you!
blake2module.c includes headers that use SIMD typedefs if an SIMD implementation will be built, but must not itself be compiled with the -m options that enable SIMD instructions. However, the *mmintrin headers are not always usable to get those typedefs if the corresponding -m option is not used.
fafe746 to 2897e64Comparejmroot commented Feb 20, 2025
The force push was just to fix a typo in the commit message. |
Uh oh!
There was an error while loading. Please reload this page.
Check for SIMD -m flags before checking for header issues. Clarify comment.
picnixz left a comment
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.
Logic wise, I think it indeed renders better now. Do we have a build bot for testing this change?
jmroot commented Feb 25, 2025
I don't think there's a buildbot that was reproducing the issue this fixes, or it would have been noticed sooner. I have locally confirmed that this fixes the build failure on some affected systems. Speaking of tests, the ones for this PR appear to have gotten stuck. Can someone manually retry them? |
picnixz commented Feb 25, 2025
I've merged main into your branch (that way it's also up-to-date) |
picnixz commented Feb 25, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I'll also run the buildbots in general so that we don't introduce some misconfiguration. In the meantime, please do not push new commits as I would need to reschedule the build bots. EDIT: Huh, apparently buildbots are not being launched =/ |
picnixz commented Feb 25, 2025
Looks like there are some network failures:
|
chris-eibl commented Feb 25, 2025
Just for visibility here: @msprotz is working on hiding the "problematic"
Maybe this temporary workaround is not needed for the alphas? From https://peps.python.org/pep-0745/ |
msprotz commented Feb 25, 2025
alpha 7 is reasonable, alpha 6 is maybe -- does that sound right? |
picnixz commented Feb 25, 2025
Depending on how the fix for libintvector turns out to be, it could help a lot the HMAC PR (#130157) that I want to add to the alpha. We only have until May for that because past this point, no features are accepted. And I'd like to include built-in HMAC if possible. If it can solve at the same time this and my issue, then I'd say it should be fixed ASAP. However, I can also live without a better fix for libintvector. In this case, we can merge this fix and I'll try the alternative I was told (I still didn't have time to come back to that PR though) |
msprotz commented Feb 27, 2025
I thought it'd be more productive if I tried to push a little on this while we have it all fresh in our heads. https://github.com/hacl-star/hacl-star/compare/protz_abstract_struct please check this branch out -- none of the public headers include libintvector.h can you confirm that in its current state, this branch works and eliminates the need for ugly workarounds? it might even eliminate the need to have the pesky configure check in #130213 and #130332 |
chris-eibl commented Feb 27, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I gave it a quick try, but the "reorganizations" happening in refresh.sh made it less quick :) If I didn't mess up, this seems good to me except some missing This is because now (even on hacl-star main) files like With those guards applied locally, I was able to compile with clang-cl 18.1.8, which previously failed due to the intrinsics :) |
chris-eibl commented Mar 16, 2025
jmroot commented Mar 22, 2025
The updated HACL* now in main does indeed seem to do the trick, so this is not needed. |
blake2module.cincludes headers that use SIMD typedefs if an SIMD implementation will be built, but must not itself be compiled with the-moptions that enable SIMD instructions. However, the *mmintrin headers are not always usable to get those typedefs if the corresponding-moption is not used.