[SystemZ] The Local Area Offset in SystemZFrameLowering.

[jonpa]

Summary

I began looking at how to implement the packed stack layout and got kind of stuck on understanding the current code. The use of the LocalAreaOffset with a value of -160 does not seem to have any real benefit, although it certainly is confusing to me... Just the notion of "the local area" per

/// getOffsetOfLocalArea - This method returns the offset of the local area
/// from the stack pointer on entrance to a function.
///
int getOffsetOfLocalArea() const { return LocalAreaOffset; }

is problematic since we don't really have a single constant offset to a single area for locals. We have the register save area between 0 and +160 from EntrySP, and then we have another local area for e.g.spills, which would begin at SP+160 *after the prologue*. Incoming stack arguments would be at +160 and above from EntrySP. The use of this offset in different context is very confusing to me, and it seems much more readable to instead just allocate the stack objects with fixed offsets explicitly relative to EntrySP.

This offset is used when PEI::calculateFrameObjectOffsets() computes the offsets. This becomes -168 for the first spilled reg. TargetFrameLowering::getFrameIndexReference() then subtracts the LAO, which is negative, so we get -8 back again (plus stack size). I don't see any other uses, and it doesn't seem like we gain anything from this.

The debug dumps are currently broken, which may not be a real problem, but still:

fi#-3: size=8, align=8, fixed, at location [SP+280]
fi#-2: size=8, align=8, fixed, at location [SP+272]
fi#-1: size=8, align=8, fixed, at location [SP+160]

This is actually one incoming stack argument at +0, and then %r14 and %r15 saved at +112 and +120 while the MachineFrameInfo::print() has subtracted the LocalAreaOffset of -160 to them all.

This patch seems to work (knock-knock), by simply setting this offset to 0 and instead add a SystemZMC::RegisterSaveAreaSize constant (160) in places as needed instead.

• Removed SystemZMC::CallFrameSize - slightly confusing to me to have this as a constant since a "call frame" could also include stack arguments in my world. Use instead SystemZMC::RegisterSaveAreaSize to represent the 160-byte base area.

• All SystemZ calls to CreateFixedObject() needs to be adjusted:

LowerFormalArguments():

• incoming stack arguments needs RegisterSaveAreaSize added to be +160.
• varargs:
  • add RegisterSaveAreaSize for setVarArgsFrameIndex() to get +160 from incoming SP for first stack arg.
  • RegSaveIndex is +0 from incoming SP...
  • No need to add offset for storing of FPR to the fixed slots since the table has the correct offset from incoming SP.

lowerFRAMEADDR(): The frame address should be the incoming SP, which is also the address of the back chain, so just give 0 as offset.

• Users of getObjectOffset():

PEI::calculateFrameObjectOffsets() / MachineFrameInfo::estimateStackSize(): Should return the same value as before, since even though we now give an offset of 160 for a stack arg, this is negated and therefore not counted. This makes sense I think since it's a check into the local area to be allocated.

TargetFrameLowering::getFrameIndexReference(): As before except the offset is now included in the SP Offsets for each object while LAO is 0.

SystemZFrameLowering::processFunctionBeforeFrameFinalized(): The +160 offset is already part of the stackarg ObjectOffset so don't add it again. In fact, the current code is conservatively wrong, because it adds +160 also to the register saves, which already have the final offsets from the table into the Register save area.

SelectionDAGAddressAnalysis.cpp, in BaseIndexOffset::equalBaseIndex(): This is just a relative difference so it shouldn't matter if the LAO is included or not.

• I don't quite understand

SystemZMC::CFAOffsetFromInitialSP: A little confused as to why this has an offset from the incoming SP, as the Canonical Frame Address is supposed to be the incoming SP, per definition, or? It seems that we are pretending that the 160-byte base area is part of the call, so the CFA is above it. If I make this 0, then two DebugInfo/SystemZ/ tests fail, so it seems this is needed, unless I just forgot to update some other place as well.

the asserts in TargetInstrInfo::foldMemoryOperand() / TargetLoweringBase::emitPatchPoint():

assert(MFI.getObjectOffset(FI) != -1)

All tests pass, and SPEC is NFC except for one file where it seems that the emergency spill slots are no longer allocated (see above under processFunctionBeforeFrameFinalized()).

[uweigand]

So here is how this is supposed to work, as I understand it.

First of all, on SystemZ, unlike all other platforms, the DWARF CFA (Canonical Frame Address) is not equal to the incoming stack pointer, but to incoming stack pointer plus 160. This is simply a fact of the SystemZ ABI that we cannot change at this point.

This also affects the definition of the "getOffsetOfLocalArea" value. While the comment calls this "the offset of the local area from the stack pointer on entrance to a function", we interpret this as "the offset of the local area from the CFA" -- on all other platforms this is equivalent, but for us the difference matters. Note that since the stack grows downward, this offset describes the number of bytes we have to go downward from the CFA in order to reach the topmost byte of the local area (which is therefore simply the size of the register save area).

Before register allocation, (fixed) frame objects can be allocated relative to the CFA plus some offset. This allows reaching incoming arguments (CFA + 0 .. CFA + N) as well as the register save area (CFA ... CFA - 160). During register allocation, non-fixed frame objects are then allocated to the "local area", i.e. not within the first 160 bytes counting downwards from the CFA. In a second step, in order to establish addressability during function execution, the address is converted from "CFA - offset" into "SP (or frame pointer in case of variable stack size) + offset". Note that in that computation, the size of the other register save area (which the current function must allocate for its callees) must be included.

Given all that, I believe the SystemZ backend handles this correctly, so I don't think these changes are really necessary.

[jonpa]

Thank you very much for the explanation - it does make more sense now :-)

I think it would be nice to have this mentioned in a comment, so I tried to reuse part of your answer here in the SystemZFrameLowering() constructor.

The one part that stands out from this then to me, is the fact that the SpillOffsetTable is relative to the incoming SP, which is why the offsets are incorrect in the created stack objects. To have a fully correct IR, I guess we should return offsets relative to the CFA, in other words subtract 160 from each entry. But there is no real point in this since those offsets are never used (we compute the offset for the STMG instead), so I instead figured that at least having a comment mentioning that might be helpful.

Unfortunately I was still left with a slight confusion, as I am not quite sure how this works for the FPRs part of that table: Normally those FPRs are not callee-saved, so it wouldn't matter. But how about the CSR_SystemZ_AllRegs calling convention? If F0D would be callee-saved, would it end up in the right place?

[uweigand]

In D70427#1753150, @jonpa wrote:

The one part that stands out from this then to me, is the fact that the SpillOffsetTable is relative to the incoming SP, which is why the offsets are incorrect in the created stack objects. To have a fully correct IR, I guess we should return offsets relative to the CFA, in other words subtract 160 from each entry. But there is no real point in this since those offsets are never used (we compute the offset for the STMG instead), so I instead figured that at least having a comment mentioning that might be helpful.

Hmm, this may actually be simple to fix, have the SpillOffsetTable (which is used by common code) use offsets relative to the CFA, and convert it to offset relative to incoming SP when filling RegSpillOffsets (which is used by the SystemZ code). If this reduces confusion when reading MI dumps, this may be worthwhile to do.

Unfortunately I was still left with a slight confusion, as I am not quite sure how this works for the FPRs part of that table: Normally those FPRs are not callee-saved, so it wouldn't matter. But how about the CSR_SystemZ_AllRegs calling convention? If F0D would be callee-saved, would it end up in the right place?

F0D, F2D, F4D, F6D are used for passing floating-point arguments and must be saved into those specific slots in a function that uses variable argument lists, since the va_list holds a pointer to the start of the register save area, and va_arg assumes the first four floating-point arguments are stored at those particular offsets relative to that pointer. If you use AllRegs, they'll also be saved there, which isn't really required, but doesn't hurt either. Any other register, whether the normal call-saved register, or the other call-clobbered ones when using AllRegs, do not have assigned slots and will be saved in some arbitrary place on the stack allocated during register allocation.

[jonpa]

F0D, F2D, F4D, F6D are used for passing floating-point arguments and must be saved into those specific slots in a function that uses variable argument lists, since the va_list holds a pointer to the start of the register save area, and va_arg assumes the first four floating-point arguments are stored at those particular offsets relative to that pointer. If you use AllRegs, they'll also be saved there, which isn't really required, but doesn't hurt either. Any other register, whether the normal call-saved register, or the other call-clobbered ones when using AllRegs, do not have assigned slots and will be saved in some arbitrary place on the stack allocated during register allocation.

For variable arguments, I see that this works since we are indeed adding -160 to the offsets from the table in LowerFormalArguments().

If I compile

define anyregcc void @anyregcc1() {
entry:
  call void asm sideeffect "", "~{f0}"() nounwind
  ret void
}

I get

anyregcc1:                              # @anyregcc1
        .cfi_startproc
# %bb.0:                                # %entry
        std     %f0, 288(%r15)          # 8-byte Folded Spill
        .cfi_offset %f0, 128
        #APP
        #NO_APP
        ld      %f0, 288(%r15)          # 8-byte Folded Reload
        br      %r14

This seems broken to me since the correct offset for %f0 from the incoming SP is 128, or?

[uweigand]

Right. It seems we really do need to implement the change I described in my last comment then. I've just quickly tried this patch:

diff-spilloffsettable2 KBDownload

and this seems to fix the problem for your test case:

anyregcc1:                              # @anyregcc1
        .cfi_startproc
# %bb.0:                                # %entry
        std     %f0, 128(%r15)          # 8-byte Folded Spill
        .cfi_offset %f0, -32
        #APP
        #NO_APP
        ld      %f0, 128(%r15)          # 8-byte Folded Reload
        br      %r14

Can you check whether this doesn't introduce any regressions anywhere, and integrate your comment and test case? Thanks!

[jonpa]

Applied your patch, which is NFC on SPEC. Fixed your comment above SpillOffsetTable slightly to "..., relative to the CFA (i.e. incoming stack pointer + SystemZMC::CallFrameSize)".

I extended llvm/test/CodeGen/SystemZ/anyregcc-novec.ll to also check for the actual offsets for the FP argument registers. The cfi_offset:s for these registers have now also changed, and I added them as well to the test.

[uweigand]

Minor suggestion on the test case, otherwise this LGTM. Thanks!

llvm/test/CodeGen/SystemZ/anyregcc-novec.ll

7 ↗

(On Diff #230918)

If we check absolute stack offsets below, we should probably add a check for the instruction that decrements the stack pointer here as well (the aghi %r15, -...).

[jonpa]

Committed as a7d3f69.

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.h

 class Target;
 class Triple;
 class raw_pwrite_stream;
 class raw_ostream;
 
 namespace SystemZMC {
 // How many bytes are in the ABI-defined, caller-allocated part of
 // a stack frame.
-const int64_t CallFrameSize = 160;
+const int64_t RegisterSaveAreaSize = 160;
 
 // The offset of the DWARF CFA from the incoming stack pointer.
-const int64_t CFAOffsetFromInitialSP = CallFrameSize;
+const int64_t CFAOffsetFromInitialSP = RegisterSaveAreaSize;
 
 // Maps of asm register numbers to LLVM register numbers, with 0 indicating
 // an invalid register.  In principle we could use 32-bit and 64-bit register
 // classes directly, provided that we relegated the GPR allocation order
 // in SystemZRegisterInfo.td to an AltOrder and left the default order
 // as %r0-%r15.  It seems better to provide the same interface for
 // all classes though.
 extern const unsigned GR32Regs[16];

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

static const TargetFrameLowering::SpillSlot SpillOffsetTable[] = {
   { SystemZ::F2D,  0x88 },
   { SystemZ::F4D,  0x90 },
   { SystemZ::F6D,  0x98 }
 };
 } // end anonymous namespace
 
 SystemZFrameLowering::SystemZFrameLowering()
     : TargetFrameLowering(TargetFrameLowering::StackGrowsDown, Align(8),
-                          -SystemZMC::CallFrameSize, Align(8),
-                          false /* StackRealignable */) {
+                          0, Align(8), false /* StackRealignable */) {
   // Create a mapping from register number to save slot offset.
   RegSpillOffsets.grow(SystemZ::NUM_TARGET_REGS);
   for (unsigned I = 0, E = array_lengthof(SpillOffsetTable); I != E; ++I)
     RegSpillOffsets[SpillOffsetTable[I].Reg] = SpillOffsetTable[I].Offset;
 }
 
 const TargetFrameLowering::SpillSlot *
 SystemZFrameLowering::getCalleeSavedSpillSlots(unsigned &NumEntries) const {

 }
 
 void SystemZFrameLowering::
 processFunctionBeforeFrameFinalized(MachineFunction &MF,
                                     RegScavenger *RS) const {
   MachineFrameInfo &MFFrame = MF.getFrameInfo();
   // Get the size of our stack frame to be allocated ...
   uint64_t StackSize = (MFFrame.estimateStackSize(MF) +
-                        SystemZMC::CallFrameSize);
+                        SystemZMC::RegisterSaveAreaSize);
   // ... and the maximum offset we may need to reach into the
   // caller's frame to access the save area or stack arguments.
-  int64_t MaxArgOffset = SystemZMC::CallFrameSize;
+  int64_t MaxArgOffset = SystemZMC::RegisterSaveAreaSize;
   for (int I = MFFrame.getObjectIndexBegin(); I != 0; ++I)
     if (MFFrame.getObjectOffset(I) >= 0) {
-      int64_t ArgOffset = SystemZMC::CallFrameSize +
-                          MFFrame.getObjectOffset(I) +
-                          MFFrame.getObjectSize(I);
+      int64_t ArgOffset = MFFrame.getObjectOffset(I) + MFFrame.getObjectSize(I);
       MaxArgOffset = std::max(MaxArgOffset, ArgOffset);
     }
 
   uint64_t MaxReach = StackSize + MaxArgOffset;
   if (!isUInt<12>(MaxReach)) {
     // We may need register scavenging slots if some parts of the frame
     // are outside the reach of an unsigned 12-bit displacement.
     // Create 2 for the case where both addresses in an MVC are
if (MF.getFunction().getCallingConv() == CallingConv::GHC) {
     if (MFFrame.getStackSize() > 2048 * sizeof(long)) {
       report_fatal_error(
           "Pre allocated stack space for GHC function is too small");
     }
     if (HasFP) {
       report_fatal_error(
           "In GHC calling convention a frame pointer is not supported");
     }
-    MFFrame.setStackSize(MFFrame.getStackSize() + SystemZMC::CallFrameSize);
+    MFFrame.setStackSize(MFFrame.getStackSize() +
+                         SystemZMC::RegisterSaveAreaSize);
     return;
   }
 
   // Debug location must be unknown since the first debug location is used
   // to determine the end of the prologue.
   DebugLoc DL;
 
   // The current offset of the stack pointer from the CFA.
if (ZFI->getLowSavedGPR()) {
     }
   }
 
   uint64_t StackSize = MFFrame.getStackSize();
   // We need to allocate the ABI-defined 160-byte base area whenever
   // we allocate stack space for our own use and whenever we call another
   // function.
   if (StackSize || MFFrame.hasVarSizedObjects() || MFFrame.hasCalls()) {
-    StackSize += SystemZMC::CallFrameSize;
+    StackSize += SystemZMC::RegisterSaveAreaSize;
     MFFrame.setStackSize(StackSize);
   }
 
   if (StackSize) {
     // Determine if we want to store a backchain.
     bool StoreBackchain = MF.getFunction().hasFnAttribute("backchain");
 
     // If we need backchain, save current stack pointer.  R1 is free at this
if (StackSize) {
     unsigned CFIIndex = MF.addFrameInst(
         MCCFIInstruction::createDefCfaOffset(nullptr, SPOffsetFromCFA + Delta));
     BuildMI(MBB, MBBI, DL, ZII->get(TargetOpcode::CFI_INSTRUCTION))
         .addCFIIndex(CFIIndex);
     SPOffsetFromCFA += Delta;
 
     if (StoreBackchain)
       BuildMI(MBB, MBBI, DL, ZII->get(SystemZ::STG))
-        .addReg(SystemZ::R1D, RegState::Kill).addReg(SystemZ::R15D).addImm(0).addReg(0);
+        .addReg(SystemZ::R1D, RegState::Kill).addReg(SystemZ::R15D).addImm(0)
+        .addReg(0);
   }
 
   if (HasFP) {
     // Copy the base of the frame to R11.
     BuildMI(MBB, MBBI, DL, ZII->get(SystemZ::LGR), SystemZ::R11D)
       .addReg(SystemZ::R15D);
 
     // Add CFI for the new frame location.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

if (VA.isRegLoc()) {
       }
 
       Register VReg = MRI.createVirtualRegister(RC);
       MRI.addLiveIn(VA.getLocReg(), VReg);
       ArgValue = DAG.getCopyFromReg(Chain, DL, VReg, LocVT);
     } else {
       assert(VA.isMemLoc() && "Argument not register or memory");
 
-      // Create the frame index object for this incoming parameter.
+      // Create the frame index object for this incoming parameter. These
+      // parameters live just above the register save area.
       int FI = MFI.CreateFixedObject(LocVT.getSizeInBits() / 8,
-                                     VA.getLocMemOffset(), true);
+                 SystemZMC::RegisterSaveAreaSize + VA.getLocMemOffset(), true);
 
       // Create the SelectionDAG nodes corresponding to a load
       // from this parameter.  Unpromoted ints and floats are
       // passed as right-justified 8-byte values.
       SDValue FIN = DAG.getFrameIndex(FI, PtrVT);
       if (VA.getLocVT() == MVT::i32 || VA.getLocVT() == MVT::f32)
         FIN = DAG.getNode(ISD::ADD, DL, PtrVT, FIN,
                           DAG.getIntPtrConstant(4, DL));
SDValue SystemZTargetLowering::LowerFormalArguments(
   if (IsVarArg) {
     // Save the number of non-varargs registers for later use by va_start, etc.
     FuncInfo->setVarArgsFirstGPR(NumFixedGPRs);
     FuncInfo->setVarArgsFirstFPR(NumFixedFPRs);
 
     // Likewise the address (in the form of a frame index) of where the
     // first stack vararg would be.  The 1-byte size here is arbitrary.
     int64_t StackSize = CCInfo.getNextStackOffset();
-    FuncInfo->setVarArgsFrameIndex(MFI.CreateFixedObject(1, StackSize, true));
+    FuncInfo->setVarArgsFrameIndex(MFI.CreateFixedObject(1,
+                            StackSize + SystemZMC::RegisterSaveAreaSize, true));
 
     // ...and a similar frame index for the caller-allocated save area
     // that will be used to store the incoming registers.
-    int64_t RegSaveOffset = TFL->getOffsetOfLocalArea();
-    unsigned RegSaveIndex = MFI.CreateFixedObject(1, RegSaveOffset, true);
+    unsigned RegSaveIndex = MFI.CreateFixedObject(1, 0, true);
     FuncInfo->setRegSaveFrameIndex(RegSaveIndex);
 
     // Store the FPR varargs in the reserved frame slots.  (We store the
     // GPRs as part of the prologue.)
     if (NumFixedFPRs < SystemZ::NumArgFPRs) {
       SDValue MemOps[SystemZ::NumArgFPRs];
       for (unsigned I = NumFixedFPRs; I < SystemZ::NumArgFPRs; ++I) {
         unsigned Offset = TFL->getRegSpillOffset(SystemZ::ArgFPRs[I]);
-        int FI = MFI.CreateFixedObject(8, RegSaveOffset + Offset, true);
+        int FI = MFI.CreateFixedObject(8, Offset, true);
         SDValue FIN = DAG.getFrameIndex(FI, getPointerTy(DAG.getDataLayout()));
         unsigned VReg = MF.addLiveIn(SystemZ::ArgFPRs[I],
                                      &SystemZ::FP64BitRegClass);
         SDValue ArgValue = DAG.getCopyFromReg(Chain, DL, VReg, MVT::f64);
         MemOps[I] = DAG.getStore(ArgValue.getValue(1), DL, ArgValue, FIN,
                                  MachinePointerInfo::getFixedStack(MF, FI));
       }
       // Join the stores, which are independent of one another.
if (VA.isRegLoc())
       RegsToPass.push_back(std::make_pair(VA.getLocReg(), ArgValue));
     else {
       assert(VA.isMemLoc() && "Argument not register or memory");
 
       // Work out the address of the stack slot.  Unpromoted ints and
       // floats are passed as right-justified 8-byte values.
       if (!StackPtr.getNode())
         StackPtr = DAG.getCopyFromReg(Chain, DL, SystemZ::R15D, PtrVT);
-      unsigned Offset = SystemZMC::CallFrameSize + VA.getLocMemOffset();
+      unsigned Offset = SystemZMC::RegisterSaveAreaSize + VA.getLocMemOffset();
       if (VA.getLocVT() == MVT::i32 || VA.getLocVT() == MVT::f32)
         Offset += 4;
       SDValue Address = DAG.getNode(ISD::ADD, DL, PtrVT, StackPtr,
                                     DAG.getIntPtrConstant(Offset, DL));
 
       // Emit the store.
       MemOpChains.push_back(
           DAG.getStore(Chain, DL, ArgValue, Address, MachinePointerInfo()));
SDValue SystemZTargetLowering::lowerFRAMEADDR(SDValue Op,
   unsigned Depth = cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue();
   EVT PtrVT = getPointerTy(DAG.getDataLayout());
 
   // If the back chain frame index has not been allocated yet, do so.
   SystemZMachineFunctionInfo *FI = MF.getInfo<SystemZMachineFunctionInfo>();
   int BackChainIdx = FI->getFramePointerSaveIndex();
   if (!BackChainIdx) {
     // By definition, the frame address is the address of the back chain.
-    BackChainIdx = MFI.CreateFixedObject(8, -SystemZMC::CallFrameSize, false);
+    BackChainIdx = MFI.CreateFixedObject(8, 0, false);
     FI->setFramePointerSaveIndex(BackChainIdx);
   }
   SDValue BackChain = DAG.getFrameIndex(BackChainIdx, PtrVT);
 
   // FIXME The frontend should detect this case.
   if (Depth > 0) {
     report_fatal_error("Unsupported stack frame traversal count");
   }

llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp

 // Split ADJDYNALLOC instruction MI.
 void SystemZInstrInfo::splitAdjDynAlloc(MachineBasicBlock::iterator MI) const {
   MachineBasicBlock *MBB = MI->getParent();
   MachineFunction &MF = *MBB->getParent();
   MachineFrameInfo &MFFrame = MF.getFrameInfo();
   MachineOperand &OffsetMO = MI->getOperand(2);
 
   uint64_t Offset = (MFFrame.getMaxCallFrameSize() +
-                     SystemZMC::CallFrameSize +
+                     SystemZMC::RegisterSaveAreaSize +
                      OffsetMO.getImm());
   unsigned NewOpcode = getOpcodeForOffset(SystemZ::LA, Offset);
   assert(NewOpcode && "No support for huge argument lists yet");
   MI->setDesc(get(NewOpcode));
   OffsetMO.setImm(Offset);
 }
 
 // MI is an RI-style pseudo instruction.  Replace it with LowOpcode