gh-130213: Check availability of Intel SIMD types#130332
Conversation
msprotz
left a comment
There was a problem hiding this comment.
This is as discussed and matches my understanding of the problem. Thank you!
Sorry, something went wrong.
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
2897e64
Compare
February 20, 2025 21:22
|
The force push was just to fix a typo in the commit message. |
Sorry, something went wrong.
Check for SIMD -m flags before checking for header issues. Clarify comment.
picnixz
left a comment
There was a problem hiding this comment.
Logic wise, I think it indeed renders better now. Do we have a build bot for testing this change?
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
I've merged main into your branch (that way it's also up-to-date) |
Sorry, something went wrong.
|
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 =/ |
Sorry, something went wrong.
|
Looks like there are some network failures:
|
Sorry, something went wrong.
|
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/ |
Sorry, something went wrong.
|
alpha 7 is reasonable, alpha 6 is maybe -- does that sound right? |
Sorry, something went wrong.
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) |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
|
The updated HACL* now in main does indeed seem to do the trick, so this is not needed. |
Sorry, something went wrong.
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.