Skip to content

Add platform-specific configurations for iOS and macOS ARM64 to prevent AVX code compilation on x64 macOS #6731

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

Closed
wants to merge 2 commits into from

Conversation

AkiSakurai
Copy link
Contributor

…nt AVX code compilation on x64 macOS

- Updated `Utils.cmake` to set appropriate `target_platforms_arg` for iOS and macOS when building on ARM64.
- Added execution platforms `ios-arm64` and `macos-arm64` in `shim/BUCK` for specifying CPU and OS configurations.
- fix pytorch#6691
Copy link

pytorch-bot bot commented Nov 8, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/6731

Note: Links to docs will display an error until the docs builds have been completed.

❗ 2 Active SEVs

There are 2 currently active SEVs. If your PR is affected, please view them below:

❌ 3 New Failures

As of commit 4c1d38f with merge base 39e5b91 (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 8, 2024
@JacobSzwejbka
Copy link
Contributor

@helunwencser @GregoryComer @larryliu0820 can you take a look

@larryliu0820
Copy link
Contributor

Thanks for fixing the issue! Can you please add a unit test / CI job for this? Or maybe list out the repro commands so someone else can add the test.

@zcbenz
Copy link

zcbenz commented Nov 15, 2024

@AkiSakurai I think I have met the opposite issue with yours in #6839, do you have an idea how to make cross-compilation to x64 from arm64 work? I would be happy to test.

@zcbenz
Copy link

zcbenz commented Nov 15, 2024

Actually I tried apply this patch and #6839 also gets fixed with it:

diff --git a/build/Utils.cmake b/build/Utils.cmake
index bdda2b220..d81a2c8ef 100644
--- a/build/Utils.cmake
+++ b/build/Utils.cmake
@@ -206,6 +206,8 @@ function(extract_sources sources_file)
     if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
       if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
         set(target_platforms_arg "--target-platforms=shim//:macos-arm64")
+      elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64")
+        set(target_platforms_arg "--target-platforms=shim//:macos-x86_64")
       endif()
     endif()
 
diff --git a/shim/BUCK b/shim/BUCK
index c4b0c91a0..fa0d8464b 100644
--- a/shim/BUCK
+++ b/shim/BUCK
@@ -90,3 +90,11 @@ execution_platform(
     use_windows_path_separators = False,
     visibility = ["PUBLIC"],
 )
+
+execution_platform(
+    name = "macos-x86_64",
+    cpu_configuration = "prelude//cpu:x86_64",
+    os_configuration = "prelude//os:macos",
+    use_windows_path_separators = False,
+    visibility = ["PUBLIC"],
+)

Maybe include it in this PR?

@AkiSakurai
Copy link
Contributor Author

Thanks for fixing the issue! Can you please add a unit test / CI job for this? Or maybe list out the repro commands so someone else can add the test.

To reproduce, run ./build/build_apple_frameworks.sh --coreml --custom --mps --optimized --portable --quantized --xnnpack on intel mac.

@shoumikhin
Copy link
Contributor

@AkiSakurai do we still need this change? Please make sure the tests pass.

@AkiSakurai
Copy link
Contributor Author

@AkiSakurai do we still need this change? Please make sure the tests pass.

No, it was fixed in #8346

@byjlw byjlw closed this Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
7 participants