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

[LTO] Set default Darwin CPU to apple-a7 for AArch64 #81540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AtariDreams
Copy link
Contributor

Use apple-a7 as default for AArch64 Darwin targets that are not arm64e, as apple-a7 is the first AArch64 CPU that can run Darwin.

@llvmbot llvmbot added the LTO Link time optimization (regular/full LTO or ThinLTO) label Feb 12, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 12, 2024

@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-llvm-globalisel
@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-lto

Author: AtariDreams (AtariDreams)

Changes

Use apple-a7 as default for AArch64 Darwin targets that are not arm64e, as apple-a7 is the first AArch64 CPU that can run Darwin.


Full diff: https://github.com/llvm/llvm-project/pull/81540.diff

2 Files Affected:

  • (modified) llvm/lib/LTO/LTOCodeGenerator.cpp (+3-2)
  • (modified) llvm/lib/LTO/ThinLTOCodeGenerator.cpp (+5-2)
diff --git a/llvm/lib/LTO/LTOCodeGenerator.cpp b/llvm/lib/LTO/LTOCodeGenerator.cpp
index 52d8fff14be9ce..93fdde9c9b1d97 100644
--- a/llvm/lib/LTO/LTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -417,8 +417,9 @@ bool LTOCodeGenerator::determineTarget() {
       Config.CPU = "yonah";
     else if (Triple.isArm64e())
       Config.CPU = "apple-a12";
-    else if (Triple.getArch() == llvm::Triple::aarch64 ||
-             Triple.getArch() == llvm::Triple::aarch64_32)
+    else if (Triple.getArch() == llvm::Triple::aarch64)
+      Config.MCpu = "apple-a7";
+    else if (Triple.getArch() == llvm::Triple::aarch64_32)
       Config.CPU = "cyclone";
   }
 
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index 535faf5f780474..1c58cef5c3dc8d 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -546,8 +546,11 @@ static void initTMBuilder(TargetMachineBuilder &TMBuilder,
       TMBuilder.MCpu = "core2";
     else if (TheTriple.getArch() == llvm::Triple::x86)
       TMBuilder.MCpu = "yonah";
-    else if (TheTriple.getArch() == llvm::Triple::aarch64 ||
-             TheTriple.getArch() == llvm::Triple::aarch64_32)
+    else if (TheTriple.isArm64e())
+      TMBuilder.MCpu = "apple-a12";
+    else if (TheTriple.getArch() == llvm::Triple::aarch64)
+      TMBuilder.MCpu = "apple-a7";
+    else if (TheTriple.getArch() == llvm::Triple::aarch64_32)
       TMBuilder.MCpu = "cyclone";
   }
   TMBuilder.TheTriple = std::move(TheTriple);

Copy link

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

@cachemeifyoucan
Copy link
Collaborator

I think cyclone and apple-a7 is the same CPU (see AArch64.td). If we are going to rename default to a well-known name, might as well rename aarch64_32 as well.

Copy link
Contributor

@jroelofs jroelofs 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 cyclone and apple-a7 is the same CPU (see AArch64.td).

Yes.

If we are going to rename default to a well-known name, might as well rename aarch64_32 as well.

Agreed. We should prefer the apple-a* names over their weather-event-ish aliases.

else if (TheTriple.getArch() == llvm::Triple::aarch64 ||
TheTriple.getArch() == llvm::Triple::aarch64_32)
else if (TheTriple.isArm64e())
TMBuilder.MCpu = "apple-a12";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a functional change, and should come with a test.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahmedbougacha fyi, I think we're missing this one ^

Copy link
Contributor

Choose a reason for hiding this comment

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

@AtariDreams please stop resolving this

@@ -419,7 +419,7 @@ bool LTOCodeGenerator::determineTarget() {
Config.CPU = "apple-a12";
else if (Triple.getArch() == llvm::Triple::aarch64 ||
Triple.getArch() == llvm::Triple::aarch64_32)
Config.CPU = "cyclone";
Config.CPU = "apple-a7";
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that for ARM64 macOS the default CPU is apple-m1 instead. While you are here, it would probably be worth adjusting that also (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/AArch64.cpp#L50)

There probably should be a helper function to return the default CPU string and have it used at least for all llvm/lib/LTO changes, possibly also in Clang, to prevent those things from drifting apart in the future again.

Use apple-a7 as default for AArch64 Darwin targets that are not arm64e, as apple-a7 is the first AArch64 CPU that can run Darwin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants