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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added a comment.

Yeah, full disclosure: @ab, @t.p.northover and I all work at Apple.



================
Comment at: llvm/lib/Support/Host.cpp:1370
     // Default to the newest CPU we know about.
     return "apple-m1";
+#elif defined(__arm64e__)
----------------
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.


================
Comment at: llvm/lib/Support/Host.cpp:1374
+    // Apple CPU to support that ISA.
+    return "apple-a12";
+#endif
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134658