Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-123748: Add conditional compilation rules to allow HACL SIMD256 to be compiled on universal#123989
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 ourterms of service andprivacy 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
…ly ignored on universal builds.
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.
SBOM changes LGTM.
The logic looks good to me, but I wonder if this ends up enabling Blake2s/128 on ARM64? Right now it's not enabled (due to a slow barrel-shifter, I think, which gives so-so performance) by default, but might end up being enabled unintentionally for universal builds. Also, I don't know if this is specific to this work, but I tried out (not sure this is related to the problem of universal builds, or if this is a more general issue with configure) |
It will - unlike SIMD256.c, SIMD128.ccan be compiled on ARM64. However, given the existing configuration, it wouldn't be included in an ARM64-only build. I'll push an update to do a similar "#include and #undef" trick for the SIMD128 implementation so that we retain parity with single-architecture builds.
This is the same behavior I see in the official universal build of Python 3.13.0rc2 running on macOS. The same is true of other older Pythons that I've tested back to 3.8, so it doesn't appear to be specific to this patch. |
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 verified that universal2 and single-arch builds now compile without error. Thanks for tracking this down and following up.
@msprotz Are you happy with the current state of this patch? |
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 makes sense to me, thanks for untangling this one!
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.
Looks good to me, thank you for figuring out this tricky build issue -- much appreciated!
ef530ce intopython:mainUh oh!
There was an error while loading.Please reload this page.
…nd SIMD128 on macOS (python#123989)Add conditional compilation rules to allow HACL SIMD256 and SIMD128 to be ignored on the ARM64 pass of universal2 macOS builds.
Uh oh!
There was an error while loading.Please reload this page.
Add modifications to the compilation of hashlib to restore the ability to generate a universal build.
#119316 added an implementation of Blake2 to hashlib. That implementation compiles fine on single architecture macOS builds (both x86_64 and ARM64, as verified by CI); but universal2 builds generate a compilation error because the SIMD256 algorithmcan be compiled on x86_64, butcan't be compiled on ARM64. Since both architectures are compiled in a single pass, autoconf by itself can't detect or control how to include (or exclude) the SIMD256 module.
This PR:
Hacl_Hash_Blake2b_Simd256_universal2.cfile that conditionally#includesHacl_Hash_Blake2b_Simd256.c`. This keeps the Python additions isolated, allowing the original HACL sources to be used (and updated) as-is._universal2wrapper variant if compiling on universal, along with configure logging to identify when this occursblake2module.cthat disables theHACL_CAN_COMPILE_SIMD256symbol if on ARM64. On a pure ARM64 compile, this will never be executed; but in a universal build, it will.Fixes#123748.
/cc@msprotz