[RISCV] Preserve stack space for outgoing arguments when the function contain variable size objects

[shiva0217]

Summary

When the function contains variable size objects:
E.g.

bar (int x)
{

char p[x];

push outgoing variables for foo.
call foo

}

We need to generate stack adjustment instructions for outgoing arguments by
eliminateCallFramePseudoInstr when the function contains variable size
objects to avoid outgoing variables corrupt the variable size object.

Default hasReservedCallFrame will return !hasFP().
We don't want to generate extra sp adjustment instructions when hasFP()
return true, So We override hasReservedCallFrame as !hasVarSizedObjects().

[asb]

The extra sp adjustments introduced in calling-conv.ll and vararg.ll seem unnecessary - are they?

I think the test case you add could be simplified substantially, which makes the problem fixed by this patch somewhat more clear:

declare void @func(i8*, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32)       
                                                                                     
; Check that outgoing arguments passed on the stack do not corrupt a                 
; variable-sized stack object.                                                       
define void @alloca_callframe(i32 %n) nounwind {                                     
  %1 = alloca i8, i32 %n                                                             
  call void @func(i8* %1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8,           
                  i32 9, i32 10, i32 11, i32 12)                                     
  ret void                                                                           
}

[shiva0217]

Hi Alex. Default hasReservedCallFrame will return !hasFP(), so these cases will generate extra sp adjustment, we could override hasReservedCallFrame as !hasVarSizedObjects() to avoid this and thanks for update the test case which make it more clean and clear!

[asb]

In D43752#1024857, @shiva0217 wrote:

Hi Alex. Default hasReservedCallFrame will return !hasFP(), so these cases will generate extra sp adjustment, we could override hasReservedCallFrame as !hasVarSizedObjects() to avoid this and thanks for update the test case which make it more clean and clear!

That sounds like the right thing to do, and would avoid the (minor) codegen regression. Could you update to make that change? Thanks.

[shiva0217]

Hi Alex. The patch has been updated. The codgen result in calling-conv.ll:caller_many_scalars function with FP has changed due to we override the hasReservedCallFrame function. According to the logic in determineFrameLayout, the SP adjustment will change from 32 to 16. It seems that we could also remove the logic in determineFrameLayout relative to variable size objects because eliminateCallFramePseudoInstr could handle it, right?

[shiva0217]

Update test case alloca.ll

[asb]

Thanks for making those changes Shiva . The fact we can simplify determineFrameLayout is a good observation.

I've added a few comments, which I think are all comment or formatting related. With those resolved, this looks good to me.

I certainly can't see any unexpected codegen changes when compiling the GCC torture suite with this. Although I'm happy with this and happy for you to commit once resolving the formatting/comment suggestions: if anyone else following this has time to give a second look I'd appreciate it. This is a fiddly part of the codebase, so the more people checking the better.

lib/Target/RISCV/RISCVFrameLowering.cpp

235–239 ↗

(On Diff #136953)

I don't think copy+pasting the description from TargetFrameLowering.h adds much. It would be better to explain what is different about the RISC-V implementation (which is presumably that fact that there is no reserved call frame when there are varsized objects).

I know this is copied from elsewhere in LLVM, but also note that current guidance for Doxygen comments in LLVM is _not_ to repeat the function name at the beginning of the comment https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

240–241 ↗

(On Diff #136953)

Nit: clang-format is happy for this to be on one line

245 ↗

(On Diff #136953)

Nit: fill stop after comment

246–248 ↗

(On Diff #136953)

Nit: clang-format suggests the following:

MachineBasicBlock::iterator RISCVFrameLowering::eliminateCallFramePseudoInstr(
    MachineFunction &MF, MachineBasicBlock &MBB,
    MachineBasicBlock::iterator MI) const {

253–257 ↗

(On Diff #136953)

I wonder if this comment could be improved to better explain _why_ this logic is doing what it does. It's certainly true that this helps avoid poisoning of an alloca area due to outgoing arguments, but it's perhaps difficult for someone reading this code to see why. Does the following seem accurate to you?

"""
If space has not been reserved for a call frame, ADJCALLSTACKDOWN and ADJCALLSTACKUP must be converted to instructions manipulating the stack pointer. This is necessary when there is a variable length stack allocation (e.g. alloca), which means it's not possible to allocate space for outgoing arguments from within the function prologue.
"""

261–263 ↗

(On Diff #136953)

Nit: If it were me, I'd probably just write "Ensure the stack remains aligned after adjustment.", but opinions vary on what is 'obvious' and the level of detail to put in a comment - so whichever you prefer.

test/CodeGen/RISCV/alloca.ll

68–70 ↗

(On Diff #136953)

Nit: Should ideally have a new line after the declare void @func line, and these lines shouldn't be indented.

[shiva0217]

Update patch to address the comments.

[shiva0217]

Hi Alex. Yes, it's a fiddly part. I'll leave the patch to see if there have other concerned. If there're no new comments, I'll commit the patch on next Monday.

llvm/trunk/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 hasReservedCallFrame(const MachineFunction &MF) const override;
   MachineBasicBlock::iterator
   eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
-                                MachineBasicBlock::iterator MI) const override {
-    return MBB.erase(MI);
-  }
+                                MachineBasicBlock::iterator MI) const override;
 
 protected:
   const RISCVSubtarget &STI;
 
 private:
   void determineFrameLayout(MachineFunction &MF) const;
   void adjustReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                  const DebugLoc &DL, unsigned DestReg, unsigned SrcReg,
                  int64_t Val, MachineInstr::MIFlag Flag) const;
 };
 }
 #endif

llvm/trunk/lib/Target/RISCV/RISCVFrameLowering.cpp

void RISCVFrameLowering::determineFrameLayout(MachineFunction &MF) const {
 
   // 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();
 
-  // Get the maximum call frame size of all the calls.
-  uint64_t MaxCallFrameSize = MFI.getMaxCallFrameSize();
-
-  // If we have dynamic alloca then MaxCallFrameSize needs to be aligned so
-  // that allocations will be aligned.
-  if (MFI.hasVarSizedObjects())
-    MaxCallFrameSize = alignTo(MaxCallFrameSize, StackAlign);
-
-  // Update maximum call frame size.
-  MFI.setMaxCallFrameSize(MaxCallFrameSize);
-
-  // Include call frame size in total.
-  if (!(hasReservedCallFrame(MF) && MFI.adjustsStack()))
-    FrameSize += MaxCallFrameSize;
-
   // Make sure the frame is aligned.
   FrameSize = alignTo(FrameSize, StackAlign);
 
   // Update frame info.
   MFI.setStackSize(FrameSize);
 }
 
 void RISCVFrameLowering::adjustReg(MachineBasicBlock &MBB,
void RISCVFrameLowering::processFunctionBeforeFrameFinalized(
   // still needs an emergency spill slot for branch relaxation. This case
   // would currently be missed.
   if (!isInt<11>(MFI.estimateStackSize(MF))) {
     int RegScavFI = MFI.CreateStackObject(
         RegInfo->getSpillSize(*RC), RegInfo->getSpillAlignment(*RC), false);
     RS->addScavengingFrameIndex(RegScavFI);
   }
 }
+
+// Not preserve stack space within prologue for outgoing variables when the
+// function contains variable size objects and let eliminateCallFramePseudoInstr
+// preserve stack space for it.
+bool RISCVFrameLowering::hasReservedCallFrame(const MachineFunction &MF) const {
+  return !MF.getFrameInfo().hasVarSizedObjects();
+}
+
+// Eliminate ADJCALLSTACKDOWN, ADJCALLSTACKUP pseudo instructions.
+MachineBasicBlock::iterator RISCVFrameLowering::eliminateCallFramePseudoInstr(
+    MachineFunction &MF, MachineBasicBlock &MBB,
+    MachineBasicBlock::iterator MI) const {
+  unsigned SPReg = RISCV::X2;
+  DebugLoc DL = MI->getDebugLoc();
+
+  if (!hasReservedCallFrame(MF)) {
+    // If space has not been reserved for a call frame, ADJCALLSTACKDOWN and
+    // ADJCALLSTACKUP must be converted to instructions manipulating the stack
+    // pointer. This is necessary when there is a variable length stack
+    // allocation (e.g. alloca), which means it's not possible to allocate
+    // space for outgoing arguments from within the function prologue.
+    int64_t Amount = MI->getOperand(0).getImm();
+
+    if (Amount != 0) {
+      // Ensure the stack remains aligned after adjustment.
+      Amount = alignSPAdjust(Amount);
+
+      if (MI->getOpcode() == RISCV::ADJCALLSTACKDOWN)
+        Amount = -Amount;
+
+      adjustReg(MBB, MI, DL, SPReg, SPReg, Amount, MachineInstr::NoFlags);
+    }
+  }
+
+  return MBB.erase(MI);
+}

llvm/trunk/test/CodeGen/RISCV/alloca.ll

 ; RV32I-NEXT:    addi sp, sp, 16
 ; RV32I-NEXT:    ret
   %sp = call i8* @llvm.stacksave()
   %addr = alloca i8, i32 %n
   call void @notdead(i8* %addr)
   call void @llvm.stackrestore(i8* %sp)
   ret void
 }
+
+declare void @func(i8*, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32)
+
+; Check that outgoing arguments passed on the stack do not corrupt a
+; variable-sized stack object.
+define void @alloca_callframe(i32 %n) nounwind {
+; RV32I-LABEL: alloca_callframe:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    addi sp, sp, -16
+; RV32I-NEXT:    sw ra, 12(sp)
+; RV32I-NEXT:    sw s0, 8(sp)
+; RV32I-NEXT:    addi s0, sp, 16
+; 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 sp, sp, -16
+; RV32I-NEXT:    addi a1, zero, 12
+; RV32I-NEXT:    sw a1, 12(sp)
+; RV32I-NEXT:    addi a1, zero, 11
+; RV32I-NEXT:    sw a1, 8(sp)
+; RV32I-NEXT:    addi a1, zero, 10
+; RV32I-NEXT:    sw a1, 4(sp)
+; RV32I-NEXT:    addi a1, zero, 9
+; RV32I-NEXT:    sw a1, 0(sp)
+; RV32I-NEXT:    lui a1, %hi(func)
+; RV32I-NEXT:    addi t0, a1, %lo(func)
+; RV32I-NEXT:    addi a1, zero, 2
+; RV32I-NEXT:    addi a2, zero, 3
+; RV32I-NEXT:    addi a3, zero, 4
+; RV32I-NEXT:    addi a4, zero, 5
+; RV32I-NEXT:    addi a5, zero, 6
+; RV32I-NEXT:    addi a6, zero, 7
+; RV32I-NEXT:    addi a7, zero, 8
+; RV32I-NEXT:    jalr t0
+; RV32I-NEXT:    addi sp, sp, 16
+; RV32I-NEXT:    addi sp, s0, -16
+; RV32I-NEXT:    lw s0, 8(sp)
+; RV32I-NEXT:    lw ra, 12(sp)
+; RV32I-NEXT:    addi sp, sp, 16
+; RV32I-NEXT:    ret
+  %1 = alloca i8, i32 %n
+  call void @func(i8* %1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8,
+                  i32 9, i32 10, i32 11, i32 12)
+  ret void
+}

llvm/trunk/test/CodeGen/RISCV/calling-conv.ll

 ; RV32I-FPELIM-NEXT:    mv a4, zero
 ; RV32I-FPELIM-NEXT:    jalr t0
 ; RV32I-FPELIM-NEXT:    lw ra, 12(sp)
 ; RV32I-FPELIM-NEXT:    addi sp, sp, 16
 ; RV32I-FPELIM-NEXT:    ret
 ;
 ; RV32I-WITHFP-LABEL: caller_many_scalars:
 ; RV32I-WITHFP:       # %bb.0:
-; RV32I-WITHFP-NEXT:    addi sp, sp, -32
-; RV32I-WITHFP-NEXT:    sw ra, 28(sp)
-; RV32I-WITHFP-NEXT:    sw s0, 24(sp)
-; RV32I-WITHFP-NEXT:    addi s0, sp, 32
+; RV32I-WITHFP-NEXT:    addi sp, sp, -16
+; RV32I-WITHFP-NEXT:    sw ra, 12(sp)
+; RV32I-WITHFP-NEXT:    sw s0, 8(sp)
+; RV32I-WITHFP-NEXT:    addi s0, sp, 16
 ; RV32I-WITHFP-NEXT:    addi a0, zero, 8
 ; RV32I-WITHFP-NEXT:    sw a0, 4(sp)
 ; RV32I-WITHFP-NEXT:    sw zero, 0(sp)
 ; RV32I-WITHFP-NEXT:    lui a0, %hi(callee_many_scalars)
 ; RV32I-WITHFP-NEXT:    addi t0, a0, %lo(callee_many_scalars)
 ; RV32I-WITHFP-NEXT:    addi a0, zero, 1
 ; RV32I-WITHFP-NEXT:    addi a1, zero, 2
 ; RV32I-WITHFP-NEXT:    addi a2, zero, 3
 ; RV32I-WITHFP-NEXT:    addi a3, zero, 4
 ; RV32I-WITHFP-NEXT:    addi a5, zero, 5
 ; RV32I-WITHFP-NEXT:    addi a6, zero, 6
 ; RV32I-WITHFP-NEXT:    addi a7, zero, 7
 ; RV32I-WITHFP-NEXT:    mv a4, zero
 ; RV32I-WITHFP-NEXT:    jalr t0
-; RV32I-WITHFP-NEXT:    lw s0, 24(sp)
-; RV32I-WITHFP-NEXT:    lw ra, 28(sp)
-; RV32I-WITHFP-NEXT:    addi sp, sp, 32
+; RV32I-WITHFP-NEXT:    lw s0, 8(sp)
+; RV32I-WITHFP-NEXT:    lw ra, 12(sp)
+; RV32I-WITHFP-NEXT:    addi sp, sp, 16
 ; RV32I-WITHFP-NEXT:    ret
   %1 = call i32 @callee_many_scalars(i8 1, i16 2, i32 3, i64 4, i32 5, i32 6, i64 7, i32 8)
   ret i32 %1
 }
 
 ; Check passing of coerced integer arrays
 
 %struct.small = type { i32, i32* }