[RISCV] Handle variable sized objects with the stack need to be realigned

[shiva0217]

Summary

To handle the variable-sized objects with stack alignment, we need another base register(BP) to record the SP value after realignment. So we could reference the local variables after allocating the variable-sized objects by the base register. To avoid the BP value clobbered by a function call, we need to choose a callee saved register as BP. RV32E only has X8 and X9 as callee saved registers and X8 will be used as FP. So X9 will be chosen as the BP register.

[lenary]

This is looking good. Just a few small comments at the moment, and I'm going to kick off a testing run on our internal build system.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

359

Please may you use getBPReg here instead of hard-coded RISCV::X9?

At some point, we should use getFPReg for everywhere we hard-code RISCV::X2, but that's irrelevant to this change.

385

Again, use getBPReg here to avoid hard-coding RISCV::X9.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

86

Again, use getBPReg here (if you can, this one is harder)

[apazos]

Hi Shiva, I will evaluate your patch today and report back.

[apazos]

Shiva, I have tried a few workloads like EEMBC, SPEC2000/2006, perennial c++, plumhall.
They don't seem to use variable length arrays nor allocas to test this patch.
Which test suite are you using?

[shiva0217]

In D68979#1713763, @apazos wrote:

Shiva, I have tried a few workloads like EEMBC, SPEC2000/2006, perennial c++, plumhall.
They don't seem to use variable length arrays nor allocas to test this patch.
Which test suite are you using?

Hi Ana,

Thanks for the work. I ran GCC testsuite and gcc.c-torture/compile/pr51354.c will trigger the patch.
I expect rv32e would have little more cases trigger the patch due to smaller stack alignment.
But the cases of variable-sized objects with realignment are not very often.

[shiva0217]

Update patch to address the comments.

Hi @lenary, thanks for the review

[luismarques]

llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll

9

This should also be tested with FP and CFI directives.

[shiva0217]

llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll

9

For the frame contain variable-sized objects, hasFP() will return true. Ok, I'll add CFI checking line, thanks.

[shiva0217]

Add CFI checking line in the test case

[lenary]

llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll

34

This doesn't look right. Why does this directive come after the restore of s0 and s1 (but not ra)? Shouldn't it come immediately after the addi instruction?

[shiva0217]

llvm/test/CodeGen/RISCV/stack-realignment-with-variable-sized-objects.ll

34

I think the idea is that before s0 been restored, debugger could use s0 as the base to calculate CFA. After the s0 been corrupted, the CFA calculation has to base on sp with sp_offset. addi sp, s0, -128 will be generated for the frame contains variable-sized objects. For the case has FP but not contains variable-sized objects, the directive need to be inserted, too. I guess that's why the directive inserts here.

[lenary]

Please can you rebase this on top of master? @luismarques has removed CFI directives from function epilogs (because they were sometimes buggy), which should make this code simpler.

[shiva0217]

Rebase to the master

Hi @lenary, thanks for the reminding.

[lenary]

LGTM, Let's get this merged!

llvm/lib/Target/RISCV/RISCVFrameLowering.h

public:
   void determineCalleeSaves(MachineFunction &MF, BitVector &SavedRegs,
                             RegScavenger *RS) const override;
 
   void processFunctionBeforeFrameFinalized(MachineFunction &MF,
                                            RegScavenger *RS) const override;
 
   bool hasFP(const MachineFunction &MF) const override;
 
+  bool hasBP(const MachineFunction &MF) const;
+
   bool hasReservedCallFrame(const MachineFunction &MF) const override;
   MachineBasicBlock::iterator
   eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
                                 MachineBasicBlock::iterator MI) const override;
 
   // Get the first stack adjustment amount for SplitSPAdjust.
   // Return 0 if we don't want to to split the SP adjustment in prologue and
   // epilogue.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

bool RISCVFrameLowering::hasFP(const MachineFunction &MF) const {
   const TargetRegisterInfo *RegInfo = MF.getSubtarget().getRegisterInfo();
 
   const MachineFrameInfo &MFI = MF.getFrameInfo();
   return MF.getTarget().Options.DisableFramePointerElim(MF) ||
          RegInfo->needsStackRealignment(MF) || MFI.hasVarSizedObjects() ||
          MFI.isFrameAddressTaken();
 }
 
+bool RISCVFrameLowering::hasBP(const MachineFunction &MF) const {
+  const MachineFrameInfo &MFI = MF.getFrameInfo();
+  const TargetRegisterInfo *TRI = STI.getRegisterInfo();
+
+  return MFI.hasVarSizedObjects() && TRI->needsStackRealignment(MF);
+}
+
 // Determines the size of the frame and maximum call frame size.
 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();
 
void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
   assert(&MF.front() == &MBB && "Shrink-wrapping not yet supported");
 
   MachineFrameInfo &MFI = MF.getFrameInfo();
   auto *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();
   const RISCVRegisterInfo *RI = STI.getRegisterInfo();
   const RISCVInstrInfo *TII = STI.getInstrInfo();
   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");
-  }
-
   Register FPReg = getFPReg(STI);
   Register SPReg = getSPReg(STI);
+  Register BPReg = RISCVABI::getBPReg();
 
   // 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
   determineFrameLayout(MF);
 
if (RI->needsStackRealignment(MF)) {
             MF.getRegInfo().createVirtualRegister(&RISCV::GPRRegClass);
         BuildMI(MBB, MBBI, DL, TII->get(RISCV::SRLI), VR)
             .addReg(SPReg)
             .addImm(ShiftAmount);
         BuildMI(MBB, MBBI, DL, TII->get(RISCV::SLLI), SPReg)
             .addReg(VR)
             .addImm(ShiftAmount);
       }
+      // FP will be used to restore the frame in the epilogue, so we need
+      // another base register BP to record SP after re-alignment. SP will
+      // track the current stack after allocating variable sized objects.
+      if (hasBP(MF)) {
+        // move BP, SP
+        BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), BPReg)
+            .addReg(SPReg)
+            .addImm(0);
+      }
     }
   }
 }
 
 void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
                                       MachineBasicBlock &MBB) const {
   MachineBasicBlock::iterator MBBI = MBB.getLastNonDebugInstr();
   const RISCVRegisterInfo *RI = STI.getRegisterInfo();
int RISCVFrameLowering::getFrameIndexReference(const MachineFunction &MF,
 
   if (FI >= MinCSFI && FI <= MaxCSFI) {
     FrameReg = RISCV::X2;
 
     if (FirstSPAdjustAmount)
       Offset += FirstSPAdjustAmount;
     else
       Offset += MF.getFrameInfo().getStackSize();
-  } else if (RI->needsStackRealignment(MF)) {
-    assert(!MFI.hasVarSizedObjects() &&
-           "Unexpected combination of stack realignment and varsized objects");
+  } else if (RI->needsStackRealignment(MF) && !MFI.isFixedObjectIndex(FI)) {
     // If the stack was realigned, the frame pointer is set in order to allow
-    // SP to be restored, but we still access stack objects using SP.
+    // SP to be restored, so we need another base register to record the stack
+    // after realignment.
+    if (hasBP(MF))
+      FrameReg = RISCVABI::getBPReg();
+    else
       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;
 }
 
 void RISCVFrameLowering::determineCalleeSaves(MachineFunction &MF,
                                               BitVector &SavedRegs,
                                               RegScavenger *RS) const {
   TargetFrameLowering::determineCalleeSaves(MF, SavedRegs, RS);
   // Unconditionally spill RA and FP only if the function uses a frame
   // pointer.
   if (hasFP(MF)) {
     SavedRegs.set(RISCV::X1);
     SavedRegs.set(RISCV::X8);
   }
+  // Mark BP as used if function has dedicated base pointer.
+  if (hasBP(MF))
+    SavedRegs.set(RISCVABI::getBPReg());
 
   // If interrupt is enabled and there are calls in the handler,
   // unconditionally save all Caller-saved registers and
   // all FP registers, regardless whether they are used.
   MachineFrameInfo &MFI = MF.getFrameInfo();
 
   if (MF.getFunction().hasFnAttribute("interrupt") && MFI.hasCalls()) {
 

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

case RISCVABI::ABI_LP64F:
     return CSR_ILP32F_LP64F_SaveList;
   case RISCVABI::ABI_ILP32D:
   case RISCVABI::ABI_LP64D:
     return CSR_ILP32D_LP64D_SaveList;
   }
 }
 
 BitVector RISCVRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
-  const TargetFrameLowering *TFI = getFrameLowering(MF);
+  const RISCVFrameLowering *TFI = getFrameLowering(MF);
   BitVector Reserved(getNumRegs());
 
   // Use markSuperRegs to ensure any register aliases are also reserved
   markSuperRegs(Reserved, RISCV::X0); // zero
   markSuperRegs(Reserved, RISCV::X2); // sp
   markSuperRegs(Reserved, RISCV::X3); // gp
   markSuperRegs(Reserved, RISCV::X4); // tp
   if (TFI->hasFP(MF))
     markSuperRegs(Reserved, RISCV::X8); // fp
+  // Reserve the base register if we need to realign the stack and allocate
+  // variable-sized objects at runtime.
+  //
+  // To avoid the BP value clobbered by a function call, we need to choose a
+  // callee saved register to save the value. RV32E only has X8 and X9 as callee
+  // saved registers and X8 will be used as fp. So we choose X9 as bp.
+  if (TFI->hasBP(MF))
+    markSuperRegs(Reserved, RISCVABI::getBPReg()); // bp
   assert(checkAllSuperRegsMarked(Reserved));
   return Reserved;
 }
 
 bool RISCVRegisterInfo::isConstantPhysReg(unsigned PhysReg) const {
   return PhysReg == RISCV::X0;
 }
 

llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h

 //===-- RISCVBaseInfo.h - Top level definitions for RISCV MC ----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains small standalone enum definitions for the RISCV target
 // useful for the compiler back-end and the MC libraries.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVBASEINFO_H
 #define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVBASEINFO_H
 
+#include "RISCVRegisterInfo.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/MC/SubtargetFeature.h"
 
 namespace llvm {
 
 // RISCVII - This namespace holds all of the target specific flags that
enum ABI {
   ABI_Unknown
 };
 
 // Returns the target ABI, or else a StringError if the requested ABIName is
 // not supported for the given TT and FeatureBits combination.
 ABI computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
                      StringRef ABIName);
 
+// Returns the register used to hold the stack pointer after realignment.
+Register getBPReg();
+
 } // namespace RISCVABI
 
 namespace RISCVFeatures {
 
 // Validates if the given combination of features are valid for the target
 // triple. Exits with report_fatal_error if not.
 void validate(const Triple &TT, const FeatureBitset &FeatureBits);
 
 } // namespace RISCVFeatures
 
 } // namespace llvm
 
 #endif

llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.cpp

ABI computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
   // worth changing this to default to ilp32f/lp64f and ilp32d/lp64d when
   // hardware support for floating point is present.
   if (IsRV32E)
     return ABI_ILP32E;
   if (IsRV64)
     return ABI_LP64;
   return ABI_ILP32;
 }
+
+Register getBPReg() { return RISCV::X9; }
+
 } // namespace RISCVABI
 
 namespace RISCVFeatures {
 
 void validate(const Triple &TT, const FeatureBitset &FeatureBits) {
   if (TT.isArch64Bit() && FeatureBits[RISCV::FeatureRV32E])
     report_fatal_error("RV32E can't be enabled for an RV64 target");
 }
 
 } // namespace RISCVFeatures
 
 } // namespace llvm

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-with-variable-sized-objects.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(i8*, i32*)
+
+define void @caller(i32 %n) 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:    sw s1, 116(sp)
+; RV32I-NEXT:    addi s0, sp, 128
+; RV32I-NEXT:    andi sp, sp, -64
+; RV32I-NEXT:    mv s1, sp
+; RV32I-NEXT:    addi a0, a0, 15
+; RV32I-NEXT:    andi a0, a0, -16
+; RV32I-NEXT:    sub a0, sp, a0
+; RV32I-NEXT:    mv sp, a0
+; RV32I-NEXT:    addi a1, s1, 64
+; RV32I-NEXT:    call callee
+; RV32I-NEXT:    addi sp, s0, -128
+; RV32I-NEXT:    lw s1, 116(sp)
+; 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:    sd s1, 104(sp)
+; RV64I-NEXT:    addi s0, sp, 128
+; RV64I-NEXT:    andi sp, sp, -64
+; RV64I-NEXT:    mv s1, sp
+; RV64I-NEXT:    slli a0, a0, 32
+; RV64I-NEXT:    srli a0, a0, 32
+; RV64I-NEXT:    addi a0, a0, 15
+; RV64I-NEXT:    addi a1, zero, 1
+; RV64I-NEXT:    slli a1, a1, 33
+; RV64I-NEXT:    addi a1, a1, -16
+; RV64I-NEXT:    and a0, a0, a1
+; RV64I-NEXT:    sub a0, sp, a0
+; RV64I-NEXT:    mv sp, a0
+; RV64I-NEXT:    addi a1, s1, 64
+; RV64I-NEXT:    call callee
+; RV64I-NEXT:    addi sp, s0, -128
+; RV64I-NEXT:    ld s1, 104(sp)
+; RV64I-NEXT:    ld s0, 112(sp)
+; RV64I-NEXT:    ld ra, 120(sp)
+; RV64I-NEXT:    addi sp, sp, 128
+; RV64I-NEXT:    ret
+  %1 = alloca i8, i32 %n
+  %2 = alloca i32, align 64
+  call void @callee(i8* %1, i32 *%2)
+  ret void
+}