[RISCV] Fixing stack offset for RVV object with vararg in stack.

[kito-cheng]

Summary

We found LLVM generate wrong stack offset for RVV object when stack
having variable argument, that cause by we didn't count vaarg part during
calculate RVV stack objects.

Also update the stack layout diagram for including vaarg in the diagram.

Stack layout ref:
https://github.com/gcc-mirror/gcc/blob/master/gcc/config/riscv/riscv.cc#L3941

[kito-cheng]

Note, I didn't use update_mir_test_checks.py for the testcase since I try to keep stack layout in the check, and that script will generate checks for function body only.

llvm/test/CodeGen/RISCV/wrong-stack-offset-for-rvv-object.mir

23 ↗

(On Diff #420700)

See detailed stack layout diagram here.

167 ↗

(On Diff #420700)

So 40 is apparently wrong offset according the stack layout diagram.

[kito-cheng]

I guess this fix might need to consider back port LLVM 14.

[kito-cheng]

Changes:

• Move test case into test/CodeGen/RISCV/rvv.

[rogfer01]

This looks good to me, based on what we discussed on D123179 with this change the stack looks like this

The assembly emitted is as follows, the offset of v8 is now correctly computed.

asm_fprintf:                            # @asm_fprintf
# %bb.0:                                # %entry
	addi	sp, sp, -64
	sd	ra, 40(sp)                      # 8-byte Folded Spill
	sd	s0, 32(sp)                      # 8-byte Folded Spill
	sd	s1, 24(sp)                      # 8-byte Folded Spill
	csrr	a0, vlenb
	sub	sp, sp, a0
	mv	s0, a4
	mv	s1, a1
	csrr	a0, vlenb
	add	a0, sp, a0
	sd	a7, 56(a0)
	csrr	a0, vlenb
	add	a0, sp, a0
	sd	a6, 48(a0)
	vsetivli	zero, 2, e8, mf8, ta, mu
	vmv.v.i	v8, 0
	addi	a0, sp, 24
	vs1r.v	v8, (a0)                        # Unknown-size Folded Spill
.LBB0_1:                                # %while.cond
                                        # =>This Inner Loop Header: Depth=1
	bnez	zero, .LBB0_1
# %bb.2:                                # %sw.bb
                                        #   in Loop: Header=BB0_1 Depth=1
	vsetivli	zero, 2, e8, mf8, ta, mu
	addi	a0, sp, 24
	vl1r.v	v8, (a0)                        # Unknown-size Folded Reload
	vse8.v	v8, (s0)
	mv	a0, s1
	call	fprintf@plt
	j	.LBB0_1

I've also checked cases with FP and BP but they seem unaffected by the original bug.

I have no objection with this change. Thanks @kito-cheng.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

else
           StackOffset::getFixed(MFI.getStackSize() + RVFI->getRVVPadding());
   } else if (RI->hasStackRealignment(MF) && !MFI.isFixedObjectIndex(FI)) {
     // If the stack was realigned, the frame pointer is set in order to allow
     // SP to be restored, so we need another base register to record the stack
     // after realignment.
     if (hasBP(MF)) {
       FrameReg = RISCVABI::getBPReg();
       // |--------------------------| -- <-- FP
-      // | callee-saved registers   | | <----.
+      // | callee-allocated save    | | <----|
+      // | area for register varargs| |      |
+      // |--------------------------| |      |
+      // | callee-saved registers   | |      |
       // |--------------------------| --     |
       // | realignment (the size of | |      |
       // | this area is not counted | |      |
       // | in MFI.getStackSize())   | |      |
       // |--------------------------| --     |
       // | Padding after RVV        | |      |
       // | (not counted in          | |      |
       // | MFI.getStackSize())      | |      |
       // |--------------------------| --     |-- MFI.getStackSize()
       // | RVV objects              | |      |
       // | (not counted in          | |      |
       // | MFI.getStackSize())      | |      |
       // |--------------------------| --     |
       // | Padding before RVV       | |      |
       // | (not counted in          | |      |
       // | MFI.getStackSize())      | |      |
       // |--------------------------| --     |
       // | scalar local variables   | | <----'
       // |--------------------------| -- <-- BP
       // | VarSize objects          | |
       // |--------------------------| -- <-- SP
     } else {
       FrameReg = RISCV::X2;
       // |--------------------------| -- <-- FP
-      // | callee-saved registers   | | <----.
+      // | callee-allocated save    | | <----|
+      // | area for register varargs| |      |
+      // |--------------------------| |      |
+      // | callee-saved registers   | |      |
       // |--------------------------| --     |
       // | realignment (the size of | |      |
       // | this area is not counted | |      |
       // | in MFI.getStackSize())   | |      |
       // |--------------------------| --     |
       // | Padding after RVV        | |      |
       // | (not counted in          | |      |
       // | MFI.getStackSize())      | |      |
if (FI >= MinCSFI && FI <= MaxCSFI) {
     if (hasFP(MF)) {
       Offset += StackOffset::getFixed(RVFI->getVarArgsSaveSize());
       if (FI >= 0)
         Offset -= StackOffset::getFixed(RVFI->getLibCallStackSize());
       // When using FP to access scalable vector objects, we need to minus
       // the frame size.
       //
       // |--------------------------| -- <-- FP
+      // | callee-allocated save    | |
+      // | area for register varargs| |
+      // |--------------------------| |
       // | callee-saved registers   | |
       // |--------------------------| | MFI.getStackSize()
       // | scalar local variables   | |
       // |--------------------------| -- (Offset of RVV objects is from here.)
       // | RVV objects              |
       // |--------------------------|
       // | VarSize objects          |
       // |--------------------------| <-- SP
       if (MFI.getStackID(FI) == TargetStackID::ScalableVector)
         Offset -= StackOffset::getFixed(MFI.getStackSize());
     } else {
       // When using SP to access frame objects, we need to add RVV stack size.
       //
       // |--------------------------| -- <-- FP
-      // | callee-saved registers   | | <----.
+      // | callee-allocated save    | | <----|
+      // | area for register varargs| |      |
+      // |--------------------------| |      |
+      // | callee-saved registers   | |      |
       // |--------------------------| --     |
       // | Padding after RVV        | |      |
       // | (not counted in          | |      |
       // | MFI.getStackSize())      | |      |
       // |--------------------------| --     |
       // | RVV objects              | |      |-- MFI.getStackSize()
       // | (not counted in          | |      |
       // | MFI.getStackSize())      | |      |
if (hasFP(MF)) {
           Offset +=
               StackOffset::get(MFI.getStackSize() + RVFI->getRVVPadding() +
                                    RVFI->getLibCallStackSize(),
                                RVFI->getRVVStackSize());
         } else {
           Offset += StackOffset::getFixed(MFI.getStackSize());
         }
       } else if (MFI.getStackID(FI) == TargetStackID::ScalableVector) {
+        int ScalarLocalVarSize = MFI.getStackSize() -
+                                 RVFI->getCalleeSavedStackSize() -
+                                 RVFI->getVarArgsSaveSize();
         Offset += StackOffset::get(
-            alignTo(MFI.getStackSize() - RVFI->getCalleeSavedStackSize(), 8),
+            alignTo(ScalarLocalVarSize, 8),
             RVFI->getRVVStackSize());
       }
     }
   }
 
   return Offset;
 }
 

llvm/test/CodeGen/RISCV/rvv/wrong-stack-offset-for-rvv-object.mir

body:             |
   ; CHECK-NEXT:   $x10 = PseudoReadVLENB
   ; CHECK-NEXT:   $x10 = ADD $x2, killed $x10
   ; CHECK-NEXT:   SD killed renamable $x17, killed $x10, 56 :: (store (s64))
   ; CHECK-NEXT:   $x10 = PseudoReadVLENB
   ; CHECK-NEXT:   $x10 = ADD $x2, killed $x10
   ; CHECK-NEXT:   SD killed renamable $x16, killed $x10, 48 :: (store (s64) into %fixed-stack.1, align 16)
   ; CHECK-NEXT:   dead $x0 = PseudoVSETIVLI 2, 69, implicit-def $vl, implicit-def $vtype
   ; CHECK-NEXT:   renamable $v8 = PseudoVMV_V_I_MF8 0, 2, 3, implicit $vl, implicit $vtype
-  ; CHECK-NEXT:   $x10 = ADDI $x2, 40
+  ; CHECK-NEXT:   $x10 = ADDI $x2, 24
   ; CHECK-NEXT:   PseudoVSPILL_M1 killed renamable $v8, killed $x10 :: (store unknown-size into %stack.1, align 8)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1.while.cond:
   ; CHECK-NEXT:   successors: %bb.2(0x30000000), %bb.1(0x50000000)
   ; CHECK-NEXT:   liveins: $x8, $x9
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   BNE $x0, $x0, %bb.1
   ; CHECK-NEXT:   PseudoBR %bb.2
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2.sw.bb:
   ; CHECK-NEXT:   successors: %bb.1(0x80000000)
   ; CHECK-NEXT:   liveins: $x8, $x9
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   dead $x0 = PseudoVSETIVLI 2, 69, implicit-def $vl, implicit-def $vtype
-  ; CHECK-NEXT:   $x10 = ADDI $x2, 40
+  ; CHECK-NEXT:   $x10 = ADDI $x2, 24
   ; CHECK-NEXT:   renamable $v8 = PseudoVRELOAD_M1 killed $x10 :: (load unknown-size from %stack.1, align 8)
   ; CHECK-NEXT:   PseudoVSE8_V_MF8 killed renamable $v8, renamable $x8, 2, 3, implicit $vl, implicit $vtype :: (store (s16) into %ir.0, align 1)
   ; CHECK-NEXT:   $x10 = COPY renamable $x9
   ; CHECK-NEXT:   PseudoCALL target-flags(riscv-plt) @fprintf, csr_ilp32d_lp64d, implicit-def dead $x1, implicit killed $x10, implicit-def $x2, implicit-def dead $x10
   ; CHECK-NEXT:   PseudoBR %bb.1
   bb.0.entry:
     successors: %bb.1(0x80000000)
     liveins: $x11, $x14, $x16, $x17