[RISCV] Minimal stack realignment support

[lenary]

Summary

Currently the RISC-V backend does not realign the stack. This can be an issue even for the RV32I/RV64I ABIs (where the stack is 16-byte aligned), though is rare. It will be much more comment with RV32E (though the alignment requirements for common data types remain under-documented...).

This patch adds minimal support for stack realignment. It should cope with large realignments. It will error out if the stack needs realignment and variable sized objects are present.

It feels like a lot of the code like getFrameIndexReference and determineFrameLayout could be refactored somehow, as right now it feels fiddly and brittle. We also seem to allocate a lot more memory than GCC does for equivalent C code.

[lenary]

I'm taking this off @asb's plate, because we want it to be backported to the LLVM 9.0 branch, and I have more time to investigate the issue than he does right now.

[lenary]

Rebased against master

[lenary]

Working on Stack Realignment

• Support massive realignments (hopefully not needed, but one less ICE)
• Make the tests more comprehensive

We're still allocating way more than GCC does for stack frames. I'm not sure if
this is an LLVM problem or a RISC-V backend problem, any ideas @asb?

[wwei]

@lenary @asb It seems that PEI::calculateFrameObjectOffsets function in PrologEpilogInserter pass has done some stack size rounding up work. Each stack object will call AdjustStackOffset to adjust the stack frame offset, and corresponding update may be made for MaxAlign.
Also, there's a TFI hook targetHandlesStackFrameRounding to decide if the target is responsible for rounding up the stack frame(default return false). For RISCV, maybe this function should return true I think, thus the stack size for StackRealignment will be calculated at RISCV's emitPrologue time.
For the implementation in this patch, the stack size for StackRealignment will be calculathed two times, one in PEI::calculateFrameObjectOffsets, another time in RISCVFrameLowering::determineFrameLayout, and the stack size may become bigger.)

[lenary]

Address review feedback

• implement targetHandlesStackFrameRounding to prevent stack being rounded twice.

[asb]

In D62007#1614808, @lenary wrote:

Address review feedback

• implement targetHandlesStackFrameRounding to prevent stack being rounded twice.

Could you please implement this hook in a separate patch (either a parent or child of this one). The stack allocation and prolog/epilog code is a fiddly part of the backend, where there have been bugs in the past and I'm sure there will be bugs in the future. I'd much rather separate the changes to allow easily inspection, bisection etc.

[lenary]

Undo implementation of targetHandlesStackFrameRounding

[asb]

LGTM, thanks!

Please to submit a follow-on patch for targetHandlesStackFrameRounding

Is there a reason why you want tests cases for all 32/64/128/256/512/1024/2048 alignment? It feels like 32, 2048, and 4096 would give sufficient coverage?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

221

still stack => still access stack

[asb]

Also, clang-format seems to prefer to format some of this patch differently - worth running it through again.

[lenary]

Address review feedback

• Format changes
• Reword comment

[lenary]

@hans Please may you backport this to the 9.0 branch?

[hans]

In D62007#1627422, @lenary wrote:

@hans Please may you backport this to the 9.0 branch?

Sure, merged in r368846.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

 void RISCVFrameLowering::determineFrameLayout(MachineFunction &MF) const {
   MachineFrameInfo &MFI = MF.getFrameInfo();
   const RISCVRegisterInfo *RI = STI.getRegisterInfo();
 
   // Get the number of bytes to allocate from the FrameInfo.
   uint64_t FrameSize = MFI.getStackSize();
 
   // Get the alignment.
-  uint64_t StackAlign = RI->needsStackRealignment(MF) ? MFI.getMaxAlignment()
-                                                      : getStackAlignment();
+  uint64_t StackAlign;
+  if (RI->needsStackRealignment(MF)) {
+    StackAlign = MFI.getMaxAlignment();
+    FrameSize += StackAlign;
+  } else {
+    StackAlign = getStackAlignment();
+  }
 
   // Make sure the frame is aligned.
   FrameSize = alignTo(FrameSize, StackAlign);
 
   // Update frame info.
   MFI.setStackSize(FrameSize);
 }
 

 static unsigned getSPReg(const RISCVSubtarget &STI) { return RISCV::X2; }
 
 void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
                                       MachineBasicBlock &MBB) const {
   assert(&MF.front() == &MBB && "Shrink-wrapping not yet supported");
 
   MachineFrameInfo &MFI = MF.getFrameInfo();
   auto *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();
+  const RISCVRegisterInfo *RI = STI.getRegisterInfo();
   MachineBasicBlock::iterator MBBI = MBB.begin();
 
+  if (RI->needsStackRealignment(MF) && MFI.hasVarSizedObjects()) {
+    report_fatal_error(
+        "RISC-V backend can't currently handle functions that need stack "
+        "realignment and have variable sized objects");
+  }
+
   unsigned FPReg = getFPReg(STI);
   unsigned SPReg = getSPReg(STI);
 
   // Debug location must be unknown since the first debug location is used
   // to determine the end of the prologue.
   DebugLoc DL;
 
   // Determine the correct frame layout
void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
   // registers as the frame pointer must be modified after it has been saved
   // to the stack, not before.
   // FIXME: assumes exactly one instruction is used to save each callee-saved
   // register.
   const std::vector<CalleeSavedInfo> &CSI = MFI.getCalleeSavedInfo();
   std::advance(MBBI, CSI.size());
 
   // Generate new FP.
-  if (hasFP(MF))
+  if (hasFP(MF)) {
     adjustReg(MBB, MBBI, DL, FPReg, SPReg,
               StackSize - RVFI->getVarArgsSaveSize(), MachineInstr::FrameSetup);
+
+    const RISCVRegisterInfo *RI = STI.getRegisterInfo();
+    if (RI->needsStackRealignment(MF)) {
+      unsigned MaxAlignment = MFI.getMaxAlignment();
+      if (!isInt<12>(MaxAlignment))
+        report_fatal_error(">12-bit alignment not supported");
+      const RISCVInstrInfo *TII = STI.getInstrInfo();
+      BuildMI(MBB, MBBI, DL, TII->get(RISCV::ANDI), SPReg)
+          .addReg(SPReg)
+          .addImm(-(int)MaxAlignment);
+    }
+  }
 }
 
 void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
                                       MachineBasicBlock &MBB) const {
   MachineBasicBlock::iterator MBBI = MBB.getLastNonDebugInstr();
   const RISCVRegisterInfo *RI = STI.getRegisterInfo();
   MachineFrameInfo &MFI = MF.getFrameInfo();
   auto *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();
int RISCVFrameLowering::getFrameIndexReference(const MachineFunction &MF,
   if (CSI.size()) {
     MinCSFI = CSI[0].getFrameIdx();
     MaxCSFI = CSI[CSI.size() - 1].getFrameIdx();
   }
 
   if (FI >= MinCSFI && FI <= MaxCSFI) {
     FrameReg = RISCV::X2;
     Offset += MF.getFrameInfo().getStackSize();
+  } else if (RI->needsStackRealignment(MF)) {
+    assert(!MFI.hasVarSizedObjects() &&
+           "Unexpected combination of stack realignment and varsized objects");
+    // If the stack was realigned, the frame pointer is set in order to allow
+    // SP to be restored, but we still stack objects using SP.
+    FrameReg = RISCV::X2;
+    Offset += MF.getFrameInfo().getStackSize();
   } else {
     FrameReg = RI->getFrameRegister(MF);
     if (hasFP(MF))
       Offset += RVFI->getVarArgsSaveSize();
     else
       Offset += MF.getFrameInfo().getStackSize();
   }
   return Offset;

llvm/test/CodeGen/RISCV/stack-realignment-unsupported.ll

+; RUN: not llc -mtriple=riscv32 < %s 2>&1 | FileCheck %s
+; RUN: not llc -mtriple=riscv64 < %s 2>&1 | FileCheck %s
+
+; CHECK: LLVM ERROR: RISC-V backend can't currently handle functions that need stack realignment and have variable sized objects
+
+declare void @callee(i8*, i32*)
+
+define void @caller(i32 %n) nounwind {
+  %1 = alloca i8, i32 %n
+  %2 = alloca i32, align 64
+  call void @callee(i8* %1, i32 *%2)
+  ret void
+}

llvm/test/CodeGen/RISCV/stack-realignment.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32I
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV64I
+
+declare void @callee(i32*)
+
+define void @caller() nounwind {
+; RV32I-LABEL: caller:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    addi sp, sp, -128
+; RV32I-NEXT:    sw ra, 124(sp)
+; RV32I-NEXT:    sw s0, 120(sp)
+; RV32I-NEXT:    addi s0, sp, 128
+; RV32I-NEXT:    andi sp, sp, -64
+; RV32I-NEXT:    addi a0, sp, 64
+; RV32I-NEXT:    call callee
+; RV32I-NEXT:    addi sp, s0, -128
+; RV32I-NEXT:    lw s0, 120(sp)
+; RV32I-NEXT:    lw ra, 124(sp)
+; RV32I-NEXT:    addi sp, sp, 128
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: caller:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    addi sp, sp, -128
+; RV64I-NEXT:    sd ra, 120(sp)
+; RV64I-NEXT:    sd s0, 112(sp)
+; RV64I-NEXT:    addi s0, sp, 128
+; RV64I-NEXT:    andi sp, sp, -64
+; RV64I-NEXT:    addi a0, sp, 64
+; RV64I-NEXT:    call callee
+; RV64I-NEXT:    addi sp, s0, -128
+; RV64I-NEXT:    ld s0, 112(sp)
+; RV64I-NEXT:    ld ra, 120(sp)
+; RV64I-NEXT:    addi sp, sp, 128
+; RV64I-NEXT:    ret
+  %1 = alloca i32, align 64
+  call void @callee(i32* %1)
+  ret void
+}
+
+define void @caller_no_realign() nounwind "no-realign-stack" {
+; RV32I-LABEL: caller_no_realign:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    addi sp, sp, -16
+; RV32I-NEXT:    sw ra, 12(sp)
+; RV32I-NEXT:    mv a0, sp
+; RV32I-NEXT:    call callee
+; RV32I-NEXT:    lw ra, 12(sp)
+; RV32I-NEXT:    addi sp, sp, 16
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: caller_no_realign:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    addi sp, sp, -16
+; RV64I-NEXT:    sd ra, 8(sp)
+; RV64I-NEXT:    mv a0, sp
+; RV64I-NEXT:    call callee
+; RV64I-NEXT:    ld ra, 8(sp)
+; RV64I-NEXT:    addi sp, sp, 16
+; RV64I-NEXT:    ret
+  %1 = alloca i32, align 64
+  call void @callee(i32* %1)
+  ret void
+}