[AArch64] Disable spill slot scavenging when stack realignment required.

[paulwalker-arm]

Summary

Prevent corruption of the callee-save region by spills whose offset calculation assumes FP-SP is a compile time constant.

[gberry]

Ugh, good find. Isn't this an issue if variable sized stack objects are present as well?

[MatzeB]

Huh? You cannot just disable stack slot scavenging. Who guarantees you that you don't need it?
Is the bug related to the latest changes in PEI? (I've seen some commits talking about base and frame pointers flying by but did not have time to read them or dive into the details...)

[gberry]

@MatzeB This isn't about the scavenging of a register to use for large offsets, etc., but rather the scavenging of stack slots in the CSR save area to use for regular stack objects.

[MatzeB]

Oh sorry, I didn't notice there is another "scavenging" thing in there.

[paulwalker-arm]

In D45770#1071047, @gberry wrote:

Ugh, good find. Isn't this an issue if variable sized stack objects are present as well?

The graphic (within the same file) showing the stack layout suggests a base pointer is used in such circumstances, so my guess is no.

[gberry]

In D45770#1072174, @paulwalker-arm wrote:

In D45770#1071047, @gberry wrote:

Ugh, good find. Isn't this an issue if variable sized stack objects are present as well?

The graphic (within the same file) showing the stack layout suggests a base pointer is used in such circumstances, so my guess is no.

Yeah, I took a closer look and it appears that either the BP or FP will be used in this case, which should be okay.

I also took a look at fixing this failure by using the FP as a base for cases like this, and it turned out to be not too bad:

--- a/lib/Target/AArch64/AArch64FrameLowering.cpp
+++ b/lib/Target/AArch64/AArch64FrameLowering.cpp
@@ -1019,6 +1019,13 @@ int AArch64FrameLowering::resolveFrameIndexReference(const MachineFunction &MF,
   int FPOffset = MFI.getObjectOffset(FI) + FixedObject + 16;
   int Offset = MFI.getObjectOffset(FI) + MFI.getStackSize();
   bool isFixed = MFI.isFixedObjectIndex(FI);
+  bool isCSR = !isFixed && MFI.getObjectOffset(FI) >=
+                               -((int)AFI->getCalleeSavedStackSize());

   // Use frame pointer to reference fixed objects. Use it for locals if
   // there are VLAs or a dynamically realigned SP (and thus the SP isn't
@@ -1032,6 +1034,12 @@ int AArch64FrameLowering::resolveFrameIndexReference(const MachineFunction &MF,
     // Argument access should always use the FP.
     if (isFixed) {
       UseFP = hasFP(MF);
+    } else if (isCSR && RegInfo->needsStackRealignment(MF)) {
+      // References to the CSR area must use FP if we're re-aligning the stack
+      // since the dynamically-sized alignment padding is between the SP/BP and
+      // the CSR area.
+      assert(hasFP(MF) && "Re-aligned stack must have frame pointer");
+      UseFP = true;
     } else if (hasFP(MF) && !RegInfo->needsStackRealignment(MF)) {
       // If the FPOffset is negative, we have to keep in mind that the
       // available offset range for negative offsets is smaller than for
@@ -1065,9 +1078,9 @@ int AArch64FrameLowering::resolveFrameIndexReference(const MachineFunction &MF,
     }
   }

-  assert((isFixed || !RegInfo->needsStackRealignment(MF) || !UseFP) &&
+  assert(((isFixed || isCSR) || !RegInfo->needsStackRealignment(MF) || !UseFP) &&
          "In the presence of dynamic stack pointer realignment, "
-         "non-argument objects cannot be accessed through the frame pointer");
+         "non-argument/CSR objects cannot be accessed through the frame pointer");

   if (UseFP) {
     FrameReg = RegInfo->getFrameRegister(MF);

@t.p.northover should probably approve whichever approach we take since presumably he will need to approve it for 6.0.1 anyway.

[thegameg]

I think @gberry's solution is actually the correct fix. I don't really see why we shouldn't use empty stack slots if we can.

I guess AFI->getCalleeSavedStackSize is enough to get the boundaries of the "CSR region".

[gberry]

I've put the full diff for my alternative fix here: D46063

[t.p.northover]

I think I prefer Geoff's solution too. Better to make it work than just turn off parts of the optimizer, and it's not even particularly more difficult to read.

[paulwalker-arm]

I also prefer Geoff's solution. Thanks Geoff.

lib/Target/AArch64/AArch64FrameLowering.cpp

if (BigStack) {
     }
   }
 
   // Round up to register pair alignment to avoid additional SP adjustment
   // instructions.
   AFI->setCalleeSavedStackSize(alignTo(8 * NumRegsSpilled, 16));
 }
 
+// When stack realignment is required the size of the stack frame becomes
+// runtime variable. This manifests itself as a runtime variable gap between the
+// local and callee-save regions. If FP is used to build the callee-save region
+// then FP must be used to access any locals placed within it. This does not
+// happen today causing corruption to callee saves by SP relative stores whose
+// offset is calculated assuming FP-SP is a compile time constant.
+//
+// An option is to force all callee-save region accesses to be relative to the
+// same base (SP or FP) but given the alignment is likely to reduce the benefit
+// of slot scavenging it's simpler to disable the optimisation.
 bool AArch64FrameLowering::enableStackSlotScavenging(
     const MachineFunction &MF) const {
   const AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
-  return AFI->hasCalleeSaveStackFreeSpace();
+  const AArch64RegisterInfo *RegInfo = static_cast<const AArch64RegisterInfo *>(
+      MF.getSubtarget().getRegisterInfo());
+  return AFI->hasCalleeSaveStackFreeSpace() &&
+         !RegInfo->needsStackRealignment(MF);
 }

test/CodeGen/AArch64/spill-stack-realignment.mir

+# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass=prologepilog %s -o - | FileCheck %s
+
+# Ensure we don't perform stack slot scavenging when the stack pointer must be
+# aligned to something larger than required by the target. Allowing scavenging
+# can result in the callee-save region being corrupted when FP is used to build
+# the callee-save area and SP used to spill into it.  This is becuase the value
+# of FP-SP becomes runtime variable after the alignment.
+--- |
+
+  define void @test(float* %p) nounwind {
+  entry:
+    %large_aligned_var = alloca float, align 64
+    %small_aligned_var = alloca float, align 4
+    %s0 = load float, float* %p, align 4
+    store volatile float %s0, float* %large_aligned_var, align 4
+    store volatile float %s0, float* %small_aligned_var, align 4
+    call void asm sideeffect "; inlineasm", "~{x28}"()
+    ret void
+  }
+
+...
+---
+name:            test
+tracksRegLiveness: true
+frameInfo:
+  maxAlignment:    64
+# CHECK:      stack:
+# CHECK:        name: large_aligned_var, type: default, offset: -64, size: 4,
+# CHECK-NEXT:     alignment: 64
+# CHECK-NEXT:     local-offset: -64
+# CHECK:        name: small_aligned_var, type: default, offset: -68, size: 4,
+# CHECK-NEXT:     alignment: 4
+# CHECK-NEXT:     local-offset: -68
+stack:
+  - { id: 0, name: large_aligned_var, size: 4, alignment: 64, local-offset: -64 }
+  - { id: 1, name: small_aligned_var, size: 4, alignment: 4, local-offset: -68 }
+
+# CHECK: body:
+# CHECK:   $sp = ANDXri killed ${{x[0-9]+}}, 7865
+# CHECK:   STRSui $s0, $sp, 16
+# CHECK:   STRSui killed $s0, $sp, 15
+body:             |
+  bb.0.entry:
+    liveins: $x0
+
+    $s0 = LDRSui killed $x0, 0 :: (load 4 from %ir.p)
+    STRSui $s0, %stack.0.large_aligned_var, 0 :: (volatile store 4 into %ir.large_aligned_var, align 64)
+    STRSui killed $s0, %stack.1.small_aligned_var, 0 :: (volatile store 4 into %ir.small_aligned_var)
+    ; Force preserve a CSR to create a hole in the CSR stack region.
+    INLINEASM &"; inlineasm", 1, 12, implicit-def dead early-clobber $x28
+    RET_ReallyLR
+
+...