Skip to content
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

gh-130213: Check availability of Intel SIMD types #130332

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jmroot
Copy link
Contributor

@jmroot jmroot commented Feb 20, 2025

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.

Copy link
Contributor

@msprotz msprotz left a 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!

jmroot and others added 2 commits February 21, 2025 08:20
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.
@jmroot jmroot force-pushed the check-simd-headers branch from fafe746 to 2897e64 Compare February 20, 2025 21:22
@jmroot
Copy link
Contributor Author

jmroot commented Feb 20, 2025

The force push was just to fix a typo in the commit message.

Check for SIMD -m flags before checking for header issues. Clarify
comment.
@jmroot jmroot requested a review from picnixz February 24, 2025 03:45
Copy link
Member

@picnixz picnixz left a 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
Copy link
Contributor Author

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
Copy link
Member

picnixz commented Feb 25, 2025

I've merged main into your branch (that way it's also up-to-date)

@picnixz picnixz added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 25, 2025
@picnixz
Copy link
Member

picnixz commented Feb 25, 2025

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
Copy link
Member

picnixz commented Feb 25, 2025

Looks like there are some network failures:

Cannot initiate the connection to ports.ubuntu.com:80 (2620:2d:4000:1::19). - connect (101: Network is unreachable) Cannot initiate the connection to ports.ubuntu.com:80 (2620:2d:4000:1::16). - connect (101: Network is unreachable)

@chris-eibl
Copy link
Contributor

Just for visibility here: @msprotz is working on hiding the "problematic" #include "libintvector.h" (#130483 (comment)), and asking in #130483 (comment)

Also it would help me to understand how pressing this is, so that I can prioritize the upstream work accordingly. Thank you!

Maybe this temporary workaround is not needed for the alphas?

From https://peps.python.org/pep-0745/
3.14.0 alpha 6: Friday, 2025-03-14
3.14.0 alpha 7: Tuesday, 2025-04-08

@msprotz
Copy link
Contributor

msprotz commented Feb 25, 2025

alpha 7 is reasonable, alpha 6 is maybe -- does that sound right?

@picnixz
Copy link
Member

picnixz commented Feb 25, 2025

Maybe this temporary workaround is not needed for the alphas?

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
Copy link
Contributor

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
Copy link
Contributor

chris-eibl commented Feb 27, 2025

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 HACL_CAN_COMPILE_VEC* guards around Hacl_Streaming_Blake2_Types_two_vec128_s et al in internal/Hacl_Streaming_Types.h.

This is because now (even on hacl-star main) files like Hacl_Hash_SHA*.c include internal/Hacl_Streaming_Types.h and would no longer compile without intrinsics. But that's not a big deal, I've marked the spots in your PR hacl-star/hacl-star#1025.

With those guards applied locally, I was able to compile with clang-cl 18.1.8, which previously failed due to the intrinsics :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge 🔨 test-with-buildbots Test PR w/ buildbots; report in status section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants