This is an archive of the discontinued LLVM Phabricator instance.

arm64_32: implement the desired ABI for the ILP32 triple.
AcceptedPublic

Authored by t.p.northover on Jun 11 2019, 4:22 AM.

Details

Reviewers
fhahn
jfb
Summary

This adds all of the ABI tweaks we need to match the arm64_32 ABI as it exists in the wild. Most cirtically, of course, it makes the triple ILP32. But it also covers the usual suspects when defining an ABI.

Diff Detail

Event Timeline

t.p.northover created this revision.Jun 11 2019, 4:22 AM

Updating diff. How target features are handled changed slightly upstream.

jfb accepted this revision.Nov 8 2019, 3:58 AM

Minor comments, but otherwise LGTM.

clang/lib/Basic/Targets/AArch64.cpp
167

This might affect odd non-Darwin targets? Seems unlikely, but just asking since we have existence proof with Windows that stuff is weird. Admittedly they're untested if it affects them, so I think this is fine.

clang/lib/Driver/ToolChains/Clang.cpp
5457

The comment isn't quite right anymore. Maybe don't say AArch64 since the code is obvious about what it checks?

clang/lib/Driver/ToolChains/Darwin.cpp
59

Extra space.

clang/lib/Sema/SemaChecking.cpp
5512

This is now a weird variable name, since it's aarch64 maybe 32 but not be. Could you rename IsAArch64?

clang/test/CodeGen/builtins-arm64.c
11

Why isn't this one supported?

This revision is now accepted and ready to land.Nov 8 2019, 3:58 AM
t.p.northover marked 6 inline comments as done.Nov 12 2019, 7:02 AM

Thanks, I've updated for most of the suggestions and committed it. I'll make the AArch64 naming changes separately if we decide to.

clang/lib/Basic/Targets/AArch64.cpp
167

isArch64Bit is taken directly from Triple::aarch64/Triple::aarch64_32, so I think someone would have to be intentionally bringing up an aarch64_32 platform to be affected, in which case they probably want to be.

clang/lib/Driver/ToolChains/Clang.cpp
5457

AArch64 is the official name of the 64-bit execution mode of ARM processors so I think it's still correct to say aarch64_32 is AArch64. Or were you referring to some other aspect of the comment?

clang/lib/Sema/SemaChecking.cpp
5512

I think the same point about the comment above applies here. The Triple is the odd thing out here in distinguishing the two.

clang/test/CodeGen/builtins-arm64.c
11

It was an iOS version check, I've updated the triple instead.

jfb added inline comments.Nov 13 2019, 1:56 PM
clang/lib/Driver/ToolChains/Clang.cpp
5457

It doesn't check the be variant. The comment is redundant anyways.