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

[JF Bastien via Phabricator via cfe-commits]

jfb accepted this revision.
jfb added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.

Minor comments, but otherwise LGTM.



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


================
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 &&
----------------
The comment isn't quite right anymore. Maybe don't say `AArch64` since the code is obvious about what it checks?


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:59
       .Cases("armv7s", "xscale", llvm::Triple::arm)
-      .Case("arm64", llvm::Triple::aarch64)
+      .Case("arm64",  llvm::Triple::aarch64)
+      .Case("arm64_32", llvm::Triple::aarch64_32)
----------------
Extra space.


================
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();
----------------
This is now a weird variable name, since it's aarch64 maybe 32 but not be. Could you rename `IsAArch64`?


================
Comment at: clang/test/CodeGen/builtins-arm64.c:11
 
+#if __LP64__
 void *tp (void) {
----------------
Why isn't this one supported?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63131