[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

250–254

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

255–256

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

260

Nit: fill stop after comment

261–263

Nit: clang-format suggests the following:

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

268–272

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.
"""

276–278

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

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.

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

lib/Target/RISCV/RISCVFrameLowering.cpp

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);
   }
 }
+
+/// hasReservedCallFrame - Under normal circumstances, when a frame pointer is
+/// not required, we reserve argument space for call sites in the function
+/// immediately on entry to the current function.  This eliminates the need for
+/// add/sub sp brackets around call sites.  Returns true if the call frame is
+/// included as part of the stack frame.
+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 we have alloca, convert as follows:
+    // ADJCALLSTACKDOWN -> sub, sp, sp, amount
+    // ADJCALLSTACKUP   -> add, sp, sp, amount
+    //
+    // To avoid alloca area poison by outgoing arguments.
+    int64_t Amount = MI->getOperand(0).getImm();
+
+    if (Amount != 0) {
+      // We need to keep the stack aligned properly.  To do this, we round the
+      // amount of space needed for the outgoing arguments up to the next
+      // alignment boundary.
+      Amount = alignSPAdjust(Amount);
+
+      if (MI->getOpcode() == RISCV::ADJCALLSTACKDOWN)
+        Amount = -Amount;
+
+      adjustReg(MBB, MI, DL, SPReg, SPReg, Amount, MachineInstr::NoFlags);
+    }
+  }
+
+  return MBB.erase(MI);
+}

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, -32
+; RV32I-NEXT:    sw ra, 28(sp)
+; RV32I-NEXT:    sw s0, 24(sp)
+; RV32I-NEXT:    addi s0, sp, 32
+; 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, -32
+; RV32I-NEXT:    lw s0, 24(sp)
+; RV32I-NEXT:    lw ra, 28(sp)
+; RV32I-NEXT:    addi sp, sp, 32
+; 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
+}

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* }