This is an archive of the discontinued LLVM Phabricator instance.

[Support] [BLAKE3] Fix compilation with CMAKE_OSX_ARCHITECTURES
ClosedPublic

Authored by mstorsjo on Apr 1 2022, 2:04 AM.

Details

Summary

With CMake, one can build for multiple macOS architectures
at the same time by setting CMAKE_OSX_ARCHITECTURES to multiple
architectures (avoiding needing to do two separate builds and
gluing the binaries together after the build).

In this case, while targeting x86_64 and arm64, neither IS_X64
nor IS_ARM64 is set, while compilation of the individual source
files will hit those cases (in either architecture mode).

Therefore, if we on the CMake level decide not to include the
architecture specific SIMD implementation files, also tell the
source this explicitly by passing the defines indicating that we
don't expect to use them.

Such a build clearly is less ideal than explicitly targeting one
architecture at a time if it won't include all the SIMD optimizations,
but that's a tradeoff that is up to the one deciding to do such an
universal build.

An alternative would be to include all potentially relevant source
files in the build, and wrap them in ifdefs, like #if BLAKE3_USE_NEON
within blake3_neon.c after including blake3_impl.h, or
#ifdef __x86_64__ in the .S assembly files.

Diff Detail

Event Timeline

mstorsjo created this revision.Apr 1 2022, 2:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 2:04 AM
mstorsjo requested review of this revision.Apr 1 2022, 2:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 2:04 AM
akyrtzi accepted this revision.Apr 1 2022, 10:19 AM

LGTM, but could you also include a FIXME note to indicate need to handle this kind of CMake setup for SIMD implementations, potentially using your alternative suggestion?

This revision is now accepted and ready to land.Apr 1 2022, 10:19 AM

LGTM, but could you also include a FIXME note to indicate need to handle this kind of CMake setup for SIMD implementations, potentially using your alternative suggestion?

Sure, I can add such a todo.

FWIW I also noticed that this fixes builds for i386 (for any OS I'd believe) as the blake3 C code autoenables SIMD for i386 too, but those implementations are only available via the intrinsics sources (which are trickier to hook up in cmake, which we don't enable here).

This revision was landed with ongoing or failed builds.Apr 2 2022, 2:04 PM
This revision was automatically updated to reflect the committed changes.