[PATCH] D10826: Fix AArch64 prologue for empty frame with dynamic allocas

[Kristof Beyls]

kristof.beyls added a comment.

I think the logic of this patch is roughly right, I only have a few minor comments.
After addressing those comments, this LGTM.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:352
@@ -351,3 +351,3 @@
   const unsigned Alignment = MFI->getMaxAlignment();
   const bool NeedsRealignment = (Alignment > 16);
   unsigned scratchSPReg = AArch64::SP;
----------------
This should become
  const bool NeedsRealignment = RegInfo->needsStackRealignment(MF);
That's in line with what other backends do and respects the DRY principle more.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:356-362
@@ -355,5 +355,9 @@
     // Use the first callee-saved register as a scratch register
-    assert(MF.getRegInfo().isPhysRegUsed(AArch64::X9) &&
-           "No scratch register to align SP!");
-    scratchSPReg = AArch64::X9;
+    if (MF.getRegInfo().isPhysRegUsed(AArch64::X9)) {
+      scratchSPReg = AArch64::X9;
+    } else if (RegInfo->hasBasePointer(MF)) {
+      scratchSPReg = RegInfo->getBaseRegister();
+    } else {
+      report_fatal_error("No scratch register to align SP!");
+    }
   }
----------------
I think slightly nicer/better logic is
    if (RegInfo->hasBasePointer(MF))
      scratchSPReg = RegInfo->getBaseRegister();
    else
      scratchSPReg = AArch64::X9;
    assert(MF.getRegInfo().isPhysRegUsed(scratchSPReg) &&
           "No scratch register to align SP!");

With the above code, a few small tweaks to the tests in test/CodeGen/AArch64/aarch64-dynamic-stack-layout.ll are needed (to use the base pointer X19 instead of X9 for a few tests). When both X9 and X19 are available as scratch registers it's an arbitrary decision which one to use.

================
Comment at: test/CodeGen/AArch64/aarch64-dynamic-stack-layout-pr23804.ll:1
@@ +1,2 @@
+; RUN: llc -verify-machineinstrs -mtriple=aarch64-none-linux-gnu < %s | FileCheck %s
+
----------------
I don't think there is a need to create a separate test file just for pr23804, these test cases can be added to test/CodeGen/AArch64/aarch64-dynamic-stack-layout.ll


http://reviews.llvm.org/D10826