[CodeGen] Use AdjustStackOffset for Callee Saved Registers in PEI::calculateFrameObjectOffsets

[DanielMcIntosh-IBM]

Summary

Also, changes how the CSR loop is indexed, which should avoid bugs like the one fixed by rG4a57bb5a3b74bdad9b0518009a7d7ac7ca2ac650

[arsenm]

This does look similar at first glance. What's the point of doing this? Why were these paths split in the first place?

[DanielMcIntosh-IBM]

In D120668#3349265, @arsenm wrote:

This does look similar at first glance. What's the point of doing this? Why were these paths split in the first place?

They were split when these two loops where first created in rGd31f55c2361068d71f988829d167ce9e24b8f813. I'm not sure why they weren't done as 1 loop like I have here.
It seem like the split has already caused issues with changes to one loop not getting mirrored to the other: rG4a57bb5a3b74bdad9b0518009a7d7ac7ca2ac650 added a MaxCSFrameIndex >= MinCSFrameIndex condition to the else, which was only later mirrored to the other loop by rGea0eec69f16e0f1b00fec413986e4e44f6f627fa.
Not to mention rG6893926b69088eb65053470802e00081234684a7, which hasn't yet been mirrored, and it's not clear if it should (see my comment).

The goal is to generally tidy things up, and prevent similar issues from happening in the future.

llvm/lib/CodeGen/PrologEpilogInserter.cpp

871–873

This was added by rG6893926b69088eb65053470802e00081234684a7, but only to one of the code paths, so I'm not sure if it should be added to both. @arsenm Any memory of whether there was a good reason to only add it to one half?

880–882

For CSRs, the check/update to MaxAlign by AdjustStackOffset should be redundant since rG3b065cdb64b4dcd14c43e8d4e22f2bde680740a7. I'm not sure whether I should do what I have here and assert the expected behaviour, or just pass a dummy value to AdjustStackOffset so the compiler can more easily optimize that bit out when it presumably inlines AdjustStackOffset.

llvm/lib/CodeGen/PrologEpilogInserter.cpp

for (int i = MFI.getObjectIndexBegin(); i != 0; ++i) {
     } else {
       // The maximum distance from the start pointer is at the upper
       // address of the object.
       FixedOff = MFI.getObjectOffset(i) + MFI.getObjectSize(i);
     }
     if (FixedOff > Offset) Offset = FixedOff;
   }
 
+  Align MaxAlign = MFI.getMaxAlign();
   // First assign frame offsets to stack objects that are used to spill
   // callee saved registers.
-  if (StackGrowsDown && MaxCSFrameIndex >= MinCSFrameIndex) {
-    for (unsigned i = MinCSFrameIndex; i <= MaxCSFrameIndex; ++i) {
-      if (MFI.getStackID(i) !=
-          TargetStackID::Default) // Only allocate objects on the default stack.
-        continue;
+  if (MaxCSFrameIndex >= MinCSFrameIndex) {
+    for (unsigned i = 0; i <= MaxCSFrameIndex - MinCSFrameIndex; ++i) {
+      unsigned FrameIndex =
+          StackGrowsDown ? MinCSFrameIndex + i : MaxCSFrameIndex - i;
 
-      // If the stack grows down, we need to add the size to find the lowest
-      // address of the object.
-      Offset += MFI.getObjectSize(i);
-
-      // Adjust to alignment boundary
-      Offset = alignTo(Offset, MFI.getObjectAlign(i), Skew);
-
-      LLVM_DEBUG(dbgs() << "alloc FI(" << i << ") at SP[" << -Offset << "]\n");
-      MFI.setObjectOffset(i, -Offset);        // Set the computed offset
-    }
-  } else if (MaxCSFrameIndex >= MinCSFrameIndex) {
-    // Be careful about underflow in comparisons agains MinCSFrameIndex.
-    for (unsigned i = MaxCSFrameIndex; i != MinCSFrameIndex - 1; --i) {
-      if (MFI.getStackID(i) !=
-          TargetStackID::Default) // Only allocate objects on the default stack.
+      // Only allocate objects on the default stack.
+      if (MFI.getStackID(FrameIndex) != TargetStackID::Default)
         continue;
 
-      if (MFI.isDeadObjectIndex(i))
+      // TODO: should this just be if (MFI.isDeadObjectIndex(FrameIndex))
+      if (!StackGrowsDown && MFI.isDeadObjectIndex(FrameIndex))
         continue;
 
-      // Adjust to alignment boundary
-      Offset = alignTo(Offset, MFI.getObjectAlign(i), Skew);
-
-      LLVM_DEBUG(dbgs() << "alloc FI(" << i << ") at SP[" << Offset << "]\n");
-      MFI.setObjectOffset(i, Offset);
-      Offset += MFI.getObjectSize(i);
+      AdjustStackOffset(MFI, FrameIndex, StackGrowsDown, Offset, MaxAlign,
+                        Skew);
     }
   }
 
+  assert(MaxAlign == MFI.getMaxAlign() &&
+         "MFI.getMaxAlign should already account for all callee-saved "
+         "registers without a fixed stack slot");
+
   // FixedCSEnd is the stack offset to the end of the fixed and callee-save
   // stack area.
   int64_t FixedCSEnd = Offset;
-  Align MaxAlign = MFI.getMaxAlign();
 
   // Make sure the special register scavenging spill slot is closest to the
   // incoming stack pointer if a frame pointer is required and is closer
   // to the incoming rather than the final stack pointer.
   const TargetRegisterInfo *RegInfo = MF.getSubtarget().getRegisterInfo();
   bool EarlyScavengingSlots = TFI.allocateScavengingFrameIndexesNearIncomingSP(MF);
   if (RS && EarlyScavengingSlots) {
     SmallVector<int, 2> SFIs;