This is an archive of the discontinued LLVM Phabricator instance.

Fix -mcpu=native on non-macos Apple platforms
AbandonedPublic

Authored by jroelofs on Sep 26 2022, 11:09 AM.

Details

Summary

The fallback to apple-m1 is not correct, for example, when llvm is built for
iOS hosts. When the host is ARM64e, we can do a little better, but otherwise
fall back on "generic".

Diff Detail

Event Timeline

jroelofs created this revision.Sep 26 2022, 11:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 11:09 AM
jroelofs requested review of this revision.Sep 26 2022, 11:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 11:09 AM

Apple revealed:

#define CPUFAMILY_ARM_BLIZZARD_AVALANCHE 0xda33d83d

Apple revealed:

#define CPUFAMILY_ARM_BLIZZARD_AVALANCHE 0xda33d83d

link?

I didn't see it in https://opensource.apple.com/source/xnu/xnu-7195.81.3/osfmk/mach/machine.h.auto.html

My Mac. Sorry, then it is not yet public.

ab added a comment.Sep 26 2022, 1:35 PM

Apple revealed:

#define CPUFAMILY_ARM_BLIZZARD_AVALANCHE 0xda33d83d

link?

I didn't see it in https://opensource.apple.com/source/xnu/xnu-7195.81.3/osfmk/mach/machine.h.auto.html

Note that the /source pages have been replaced by github repos (linked from the open-source portal.)
Either way, the latest xnu there still doesn't list newer CPUs that already shipped. But that's fine, we can update when the next version is out.

keith added a comment.Sep 26 2022, 4:30 PM

nice! sorry I definitely didn't consider this case, and forgot folks at apple were doing this

llvm/lib/Support/Host.cpp
1370

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!

1374

I don't fully understand the comment here, should we not be defaulting to a newer chip like a13 / a14 instead?

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

llvm/lib/Support/Host.cpp
1370

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.

1374

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.

keith added inline comments.Sep 26 2022, 5:21 PM
llvm/lib/Support/Host.cpp
1370

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

1374

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?

jroelofs added inline comments.Sep 26 2022, 5:38 PM
llvm/lib/Support/Host.cpp
1374

Oh, I hadn't considered that! Good catch, I'll update the patch.

jroelofs abandoned this revision.Sep 27 2022, 9:54 AM

On further thought, I'm the one that was confused, and @keith is right: what's there already is correct. The only difference is re: m1 vs a14, which are aliases of each other in tablegen.