[PATCH] D61259: AArch64: support arm64_32, an ILP32 slice for watchOS.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.
Herald added a subscriber: wuzish.

Thanks for upstreaming this, Tim.

At a high level, my main care about here is that what is upstreamed here doesn't conflict with also adding support at some point in the future for the AArch64 ILP32 ABI - for which a beta quality specification from Arm exists.

I browsed through the code changes. Without being a deep expert in a lot of the areas touched, I just shared a few thoughts in the few places where I saw some change that I couldn't easily come up with a plausible explanation for.

I'm assuming that there is nothing in here that would prevent adding support for non-Darwin AArch64 ILP32 later?



================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:1131-1132
   RegisterAsmPrinter<AArch64AsmPrinter> Z(getTheARM64Target());
+  RegisterAsmPrinter<AArch64AsmPrinter> W(getTheARM64_32Target());
+  RegisterAsmPrinter<AArch64AsmPrinter> V(getTheAArch64_32Target());
 }
----------------
I assume this patch introduces support for the Darwin-specific arm64_32 ILP32 ABI.
That makes me wonder why there is a need to define/create the "getTheAArch64_32Target()". Doesn't "getTheARM64_32Target()" suffice to enable support for the Darwin-specific arm64_32 ILP32 ABI?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:258-260
+  MVT getPointerTy(const DataLayout &DL, uint32_t AS = 0) const override {
+    return MVT::getIntegerVT(64);
+  }
----------------
I found it surprising that this method doesn't loop for whether ILP32 is targeted or not to decide whether the pointer type is a 32 or 64 bit integer.
Would it be worthwhile to add a small comment here why the integer pointer type is always 64bits?
Maybe I'm missing something completely trivial though...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61259