[RISCV] Fix a bug in RISCVFrameLowering.

[HsiangKai]

Summary

When we have out-going arguments passing through stack and we do not
reserve the stack space in the prologue. Use BP to access stack objects
after adjusting the stack pointer before function calls.

callseq_start -> sp = sp - reserved_space

Use FP to access fixed stack objects.
Use BP to access non-fixed stack objects.

call @foo
callseq_end -> sp = sp + reserved_space

[luismarques]

Can you describe this issue in more detail, please? I'm not following it at the moment.

We have a dynamic stack change with the sub sp, sp, a1. I don't see any more dynamic changes after that. The test case then does a bunch of BP-relative loads and stores but the BP has the same value as the SP so we could just perform SP-relative accesses... no? Was the BP meant to be copied from the SP before the dynamic SP change?

[craig.topper]

llvm/test/CodeGen/RISCV/rvv/no-reserved-frame.ll

34–36

I believe this is the incorrect calculation. It's after the addi sp, sp, -32. The 40 doesn't consider the -32 adjustment that was made.

[craig.topper]

If I understand correctly, D103622 is related as it disabled the reservedCallFrame if there is a frame pointer and vector objects.

[HsiangKai]

In D114246#3152595, @luismarques wrote:

Can you describe this issue in more detail, please? I'm not following it at the moment.

We have a dynamic stack change with the sub sp, sp, a1. I don't see any more dynamic changes after that. The test case then does a bunch of BP-relative loads and stores but the BP has the same value as the SP so we could just perform SP-relative accesses... no? Was the BP meant to be copied from the SP before the dynamic SP change?

As Craig depicted, there is an additional stack change before the function call, gfunc, to reserve a stack space for out-going arguments for the function call. After the stack changes, we should not use sp to access the stack objects. In this patch, it uses bp to access stack objects if the prologue does not preserve the space for out-going arguments and there are any out-going arguments to pass to callee.

I have a patch to add a test case to show the bug in D114245.

[frasercrmck]

LGTM other than nits. Thanks for the fix!

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

240–246

nit: outgoing

242

nit: can not use
nit: our other comments in this file don't use dollars when referencing SP/BP.

245

You don't need parens around MFI.getMaxCallFrameSize() != 0

[HsiangKai]

Update comments.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

return MF.getTarget().Options.DisableFramePointerElim(MF) ||
          RegInfo->hasStackRealignment(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->hasStackRealignment(MF);
+  // If we do not reserve stack space for outgoing arguments in prologue,
+  // we will adjust the stack pointer before call instruction. After the
+  // adjustment, we can not use SP to access the stack objects for the
+  // arguments. Instead, use BP to access these stack objects.
+  return (MFI.hasVarSizedObjects() ||
+          (!hasReservedCallFrame(MF) && MFI.getMaxCallFrameSize() != 0)) &&
+         TRI->hasStackRealignment(MF);
 }
 
 // Determines the size of the frame and maximum call frame size.
 void RISCVFrameLowering::determineFrameLayout(MachineFunction &MF) const {
   MachineFrameInfo &MFI = MF.getFrameInfo();
 
   // Get the number of bytes to allocate from the FrameInfo.
   uint64_t FrameSize = MFI.getStackSize();

llvm/test/CodeGen/RISCV/rvv/no-reserved-frame.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-v -verify-machineinstrs \
 ; RUN:   < %s | FileCheck %s
 
 define signext i32 @foo(i32 signext %aa) #0 {
 ; CHECK-LABEL: foo:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    addi sp, sp, -64
-; CHECK-NEXT:    .cfi_def_cfa_offset 64
-; CHECK-NEXT:    sd ra, 56(sp) # 8-byte Folded Spill
-; CHECK-NEXT:    sd s0, 48(sp) # 8-byte Folded Spill
+; CHECK-NEXT:    addi sp, sp, -80
+; CHECK-NEXT:    .cfi_def_cfa_offset 80
+; CHECK-NEXT:    sd ra, 72(sp) # 8-byte Folded Spill
+; CHECK-NEXT:    sd s0, 64(sp) # 8-byte Folded Spill
+; CHECK-NEXT:    sd s1, 56(sp) # 8-byte Folded Spill
 ; CHECK-NEXT:    .cfi_offset ra, -8
 ; CHECK-NEXT:    .cfi_offset s0, -16
-; CHECK-NEXT:    addi s0, sp, 64
+; CHECK-NEXT:    .cfi_offset s1, -24
+; CHECK-NEXT:    addi s0, sp, 80
 ; CHECK-NEXT:    .cfi_def_cfa s0, 0
 ; CHECK-NEXT:    csrr a1, vlenb
 ; CHECK-NEXT:    sub sp, sp, a1
 ; CHECK-NEXT:    andi sp, sp, -8
-; CHECK-NEXT:    lw t0, 36(sp)
-; CHECK-NEXT:    lw a2, 32(sp)
-; CHECK-NEXT:    lw a3, 28(sp)
-; CHECK-NEXT:    lw a4, 24(sp)
-; CHECK-NEXT:    lw a5, 20(sp)
-; CHECK-NEXT:    lw a6, 16(sp)
-; CHECK-NEXT:    lw a7, 12(sp)
-; CHECK-NEXT:    lw t1, 8(sp)
-; CHECK-NEXT:    lw t2, 4(sp)
-; CHECK-NEXT:    lw a1, 0(sp)
-; CHECK-NEXT:    sw a0, 44(sp)
-; CHECK-NEXT:    sw a0, 40(sp)
+; CHECK-NEXT:    mv s1, sp
+; CHECK-NEXT:    lw t0, 44(s1)
+; CHECK-NEXT:    lw a2, 40(s1)
+; CHECK-NEXT:    lw a3, 36(s1)
+; CHECK-NEXT:    lw a4, 32(s1)
+; CHECK-NEXT:    lw a5, 28(s1)
+; CHECK-NEXT:    lw a6, 24(s1)
+; CHECK-NEXT:    lw a7, 20(s1)
+; CHECK-NEXT:    lw t1, 16(s1)
+; CHECK-NEXT:    lw t2, 12(s1)
+; CHECK-NEXT:    lw a1, 8(s1)
+; CHECK-NEXT:    sw a0, 52(s1)
+; CHECK-NEXT:    sw a0, 48(s1)
 ; CHECK-NEXT:    addi sp, sp, -32
 ; CHECK-NEXT:    sd a1, 16(sp)
 ; CHECK-NEXT:    sd t2, 8(sp)
-; CHECK-NEXT:    addi a1, sp, 40
-; This is the incorrect calculation. It's after the addi sp, sp, -32. The 40
-; doesn't consider the -32 adjustment that was made.
+; CHECK-NEXT:    addi a1, s1, 48
 ; CHECK-NEXT:    sd t1, 0(sp)
 ; CHECK-NEXT:    mv a0, t0
 ; CHECK-NEXT:    call gfunc@plt
 ; CHECK-NEXT:    addi sp, sp, 32
 ; CHECK-NEXT:    li a0, 0
-; CHECK-NEXT:    addi sp, s0, -64
-; CHECK-NEXT:    ld ra, 56(sp) # 8-byte Folded Reload
-; CHECK-NEXT:    ld s0, 48(sp) # 8-byte Folded Reload
-; CHECK-NEXT:    addi sp, sp, 64
+; CHECK-NEXT:    addi sp, s0, -80
+; CHECK-NEXT:    ld ra, 72(sp) # 8-byte Folded Reload
+; CHECK-NEXT:    ld s0, 64(sp) # 8-byte Folded Reload
+; CHECK-NEXT:    ld s1, 56(sp) # 8-byte Folded Reload
+; CHECK-NEXT:    addi sp, sp, 80
 ; CHECK-NEXT:    ret
 entry:
   %aa.addr = alloca i32, align 4
   %local = alloca i32, align 4
   %a = alloca i32, align 4
   %b = alloca i32, align 4
   %c = alloca i32, align 4
   %d = alloca i32, align 4