[PATCH] D134658: Fix -mcpu=native on non-macos Apple platforms

[Keith Smiley via Phabricator via llvm-commits]

keith added inline comments.


================
Comment at: llvm/lib/Support/Host.cpp:1370
     // Default to the newest CPU we know about.
     return "apple-m1";
+#elif defined(__arm64e__)
----------------
jroelofs wrote:
> keith wrote:
> > we actually have a m2 definition now https://github.com/llvm/llvm-project/commit/677da09d0259d7530d32e85cb561bee15f0066e2
> > 
> > can we bump to that? Unless we're missing a constant above, but I assume we do assuming that m1 & a14 are interchange?
> > 
> > If you'd like I'm happy to do that in a later change though!
> That would be okay for mtune, but is not correct for mcpu and could lead to broken codegen when the host is an m1. For correctness, the strings returned from this function need to be conservative, not optimistic.
Similar comment to the other one, shouldn't we never hit this if the host is an M1, as it should be caught by the `CPUFAMILY_ARM_FIRESTORM_ICESTORM` above? I feel like I must be missing something obvious since this is my question in both cases


================
Comment at: llvm/lib/Support/Host.cpp:1374
+    // Apple CPU to support that ISA.
+    return "apple-a12";
+#endif
----------------
jroelofs wrote:
> keith wrote:
> > I don't fully understand the comment here, should we not be defaulting to a newer chip like a13 / a14 instead? 
> That comment explains why `apple-a12` is a better choice than `generic` when we know we're being built for an ABI that requires PAC: it is the earliest chip that supported those instructions.
> 
> If we were to put any later chip's name in here, then we could have incorrect codegen when we hit this case on A12 chips, as those arches have instructions that are not supported on A12.
> 
> Let me know if you think there's a clearer way to explain that in the comment.
I guess my thought was that we shouldn't ever be able to hit this on an actual A12 or below CPU since all of those should be handled above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134658/new/

https://reviews.llvm.org/D134658