[PATCH] D63131: arm64_32: implement the desired ABI for the ILP32 triple.

[Tim Northover via Phabricator via cfe-commits]

t.p.northover marked 6 inline comments as done.
t.p.northover added a comment.

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



================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:167
   // Target properties.
-  if (!getTriple().isOSWindows()) {
+  if (!getTriple().isOSWindows() && getTriple().isArch64Bit()) {
     Builder.defineMacro("_LP64");
----------------
jfb wrote:
> 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.
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.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5457
       // for AArch64, emit a warning and ignore the flag. Otherwise, add the
       // proper mllvm flags.
+      if (Triple.getArch() != llvm::Triple::aarch64 &&
----------------
jfb wrote:
> The comment isn't quite right anymore. Maybe don't say `AArch64` since the code is obvious about what it checks?
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?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5512
+  bool IsAArch64 = (TT.getArch() == llvm::Triple::aarch64 ||
+                    TT.getArch() == llvm::Triple::aarch64_32);
   bool IsWindows = TT.isOSWindows();
----------------
jfb wrote:
> This is now a weird variable name, since it's aarch64 maybe 32 but not be. Could you rename `IsAArch64`?
I think the same point about the comment above applies here. The Triple is the odd thing out here in distinguishing the two.


================
Comment at: clang/test/CodeGen/builtins-arm64.c:11
 
+#if __LP64__
 void *tp (void) {
----------------
jfb wrote:
> Why isn't this one supported?
It was an iOS version check, I've updated the triple instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63131