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

Enable universal binaries on Apple platforms #834

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

Conversation

jgavris
Copy link
Contributor

@jgavris jgavris commented Apr 2, 2022

  • I know this may not be desirable for development, but for releases we have a mixture of architectures in our fleet and would like to avoid Rosetta when possible.
  • Updates LLVM for:
commit 578d85e924fcb172ff9b2eaa7da6cdd6df42b020
Author: Martin Storsjö <[email protected]>
Date:   Fri Apr 1 11:50:25 2022 +0300

    [Support] [BLAKE3] Fix compilation with CMAKE_OSX_ARCHITECTURES

    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.

    This also fixes builds for i386. The blake3 source code automatically
    enables the SIMD implementations when building for i386, but we don't
    provide the sources for that build configuration.

    Differential Revision: https://reviews.llvm.org/D122884

Copy link
Collaborator

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better as an option in cmake. I don't want to saddle everyone with this change.

@jgavris
Copy link
Contributor Author

jgavris commented Apr 7, 2022

Assumed as much...I can push up another version in a little bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants