[PATCH] D35531: [COFF, ARM64] Reserve X18 register by default

[Mandeep Singh Grang via Phabricator via llvm-commits]

mgrang added inline comments.


================
Comment at: lib/Target/AArch64/AArch64Subtarget.cpp:176
+      ReserveX18(TT.isOSDarwin() ||
+                (TT.isOSWindows() && TT.isOSBinFormatCOFF())),
       IsLittle(LittleEndian), TargetTriple(TT), FrameLowering(),
----------------
mstorsjo wrote:
> The `TT.isOSBinFormatCOFF()` part feels a little superfluous here
True. It can be removed.


================
Comment at: test/CodeGen/AArch64/arm64-platform-reg.ll:1
 ; RUN: llc -mtriple=arm64-apple-ios -mattr=+reserve-x18 -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X18
 ; RUN: llc -mtriple=arm64-freebsd-gnu -mattr=+reserve-x18 -o - %s | FileCheck %s --check-prefix=CHECK-RESERVE-X18
----------------
mstorsjo wrote:
> Unrelated to this particular patch, but shouldn't we drop the `-mattr=+reserve-x18` here, to also test that darwin actuall does this right by default? (I'm not sure if there's any other tests that explicitly test that?)
I thought so too. Will fix this in another patch.


================
Comment at: test/CodeGen/AArch64/arm64-platform-reg.ll:28
+
+; CHECK-RESERVE-X18-COFF-NOT: ldr x18
+; CHECK-RESERVE-X18-COFF-NOT: str x18
----------------
mstorsjo wrote:
> Why the custom test prefix for windows/coff? Doesn't the existing CHECK-RESERVE-X18 also work for aarch64-windows?
I wasn't sure about the "ldr fp" and "Spill". Is fp an alias for x18?


Repository:
  rL LLVM

https://reviews.llvm.org/D35531