[RISCV] Fix logic for determining RVV stack padding

[frasercrmck]

Summary

We must add padding when using SP or BP to access stack objects.
Checking whether we're missing FP is not sufficient as stack realignment
uses SP too. The test in D125962 explains the specific issue in more
detail.

Split from D125787.

[reames]

LGTM, but with a requested followup/clarification in inline comment.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

948

This has a very uncomfortable interlock with getFrameIndexReference's decision making. Is there a way that we can arrange to *assert* that the decision made here about which register we'll offset from is the same as that code?

One idea: Maybe make RVVPadding an optional? So that we can distringuish between "no padding needed", and "we didn't think this was used"?

Looking at the frame index compute code, one case that worries me is the realign for non-fixed object with hasBP path. That seems to not use RVVPadding, and I can't tell if that's correct (e.g. this new code is slightly conservative) or a bug (e.g. this new code is correct, and that needs to be using padding.)

[frasercrmck]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

948

Yeah, I'm no fan of the interlock. It's already there (and I think to some extent it's always necessary) but we can at least try not making it worse. Your idea of an optional is interesting, I'll take a look at that.

About the hasBP path you mention, that does concern me too. Instinctively I think you're onto something; I actually took a look at that as part of D125787 but couldn't find a decent test case - maybe I convinced myself it was okay. I think that any such fix would be part of this patch? It's getting hard to keep track...

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

948

I'd encourage you to land this as is.

Assume for a second the realigned non-fixed objects do need offset by rvv padding. (Which, the more I think about it, the more I believe they do.)

For the realigned non-fixed object case, having padding which is incorrectly ignored causes the offsets to possibly overlap the scalar stack. I don't see that as being meaningfully worse that overlapping the CSRs (the behavior without this patch.)

Given realignment is relatively rare, I'd take spill stack corruption in rare cases over CSR corruption in all cases on vl32/64. You should definitely fix the other bug ASAP, but I don't think it needs to be a blocker here.

[frasercrmck]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

948

In the hasBP path you're mentioning, are you referring to the scalable-vector case, the Default case, or the Default + FI < 0 case? I noticed a bit too late yesterday after writing my latest comment in D125787 that the scalable-vector case is correct, I think. Rounding the offset up to 8 and adding extra padding to the size of the stack is enough, for the reasons I mentioned in that comment. In D125787 I update both scalable-vector cases to use the RVV padding rather than aligning to the next multiple of 8. Similarly, I think the Default case is fine for the same reason it's fine in the non-BP path.

The only question I have is about the FI < 0 case - I'm not sure exactly what kinds of objects might occur there because IIRC those might include incoming arguments - but shouldn't we be skipping over the entire RVV section in such a case - not just the missing padding? I've probably missed something because I'd be surprised if we've missed a bug like that for so long.

[frasercrmck]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

948

Quick update: using assert(false) it seems we have no test coverage for the stack-realigned non-fixed default FI < 0 case. I wonder if this warrants a separate patch removing it and thus bringing up discussion about what it's for?

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

948

If I'm wrapping my head around this correctly (not sure of that at all), FI < 0 objects are all supposed to be at a fixed offset from FP. So, as a result, to adjust for an SP base, we should be adding an expression which fully describes the (variable) size of the stack. We don't appear to be doing that in the untested region, and I think this is wrong.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

for (const auto &Info : MFI.getCalleeSavedInfo()) {
     int FrameIdx = Info.getFrameIdx();
     if (MFI.getStackID(FrameIdx) != TargetStackID::Default)
       continue;
 
     Size += MFI.getObjectSize(FrameIdx);
   }
   RVFI->setCalleeSavedStackSize(Size);
 
-  // Padding required to keep the RVV stack aligned to 8 bytes
-  // within the main stack. We only need this when not using FP.
-  if (RVVStackSize && !hasFP(MF) && Size % 8 != 0) {
+  // Padding required to keep the RVV stack aligned to 8 bytes within the main
+  // stack. We only need this when using SP or BP to access stack objects.
+  const TargetRegisterInfo *TRI = STI.getRegisterInfo();
+  if (RVVStackSize && (!hasFP(MF) || TRI->hasStackRealignment(MF)) &&
+      Size % 8 != 0) {
     // Because we add the padding to the size of the stack, adding
     // getStackAlign() will keep it aligned.
     RVFI->setRVVPadding(getStackAlign().value());
   }
 }
 
 static bool hasRVVFrameObject(const MachineFunction &MF) {
   // Originally, the function will scan all the stack objects to check whether

llvm/test/CodeGen/RISCV/rvv/wrong-stack-slot-rv32.mir

--- |
   ; CHECK-NEXT:    add sp, sp, a0
   ; CHECK-NEXT:    lw s9, 28(sp) # 4-byte Folded Reload
   ; CHECK-NEXT:    addi sp, sp, 32
   ; CHECK-NEXT:    ret
   entry:
     ret void
   }
 
-  ; FIXME: If the stack realignment does nothing to sp (a possibility) then
-  ; the vlenb*2-sized RVV stack object at sp+56 overlaps with the slot
-  ; allocated to spilling s9 (sp+52+vlenb*2)
   define void @rvv_clobbers_callee_save() #0 {
   ; CHECK-LABEL: rvv_clobbers_callee_save:
   ; CHECK:       # %bb.0: # %entry
-  ; CHECK-NEXT:    addi sp, sp, -64
-  ; CHECK-NEXT:    sw ra, 60(sp) # 4-byte Folded Spill
-  ; CHECK-NEXT:    sw s0, 56(sp) # 4-byte Folded Spill
-  ; CHECK-NEXT:    sw s9, 52(sp) # 4-byte Folded Spill
-  ; CHECK-NEXT:    addi s0, sp, 64
+  ; CHECK-NEXT:    addi sp, sp, -80
+  ; CHECK-NEXT:    sw ra, 76(sp) # 4-byte Folded Spill
+  ; CHECK-NEXT:    sw s0, 72(sp) # 4-byte Folded Spill
+  ; CHECK-NEXT:    sw s9, 68(sp) # 4-byte Folded Spill
+  ; CHECK-NEXT:    addi s0, sp, 80
   ; CHECK-NEXT:    csrr a1, vlenb
   ; CHECK-NEXT:    slli a1, a1, 1
   ; CHECK-NEXT:    sub sp, sp, a1
   ; CHECK-NEXT:    andi sp, sp, -32
   ; CHECK-NEXT:    sw a0, 32(sp) # 4-byte Folded Spill
   ; CHECK-NEXT:    addi a0, sp, 56
   ; CHECK-NEXT:    vs2r.v v30, (a0) # Unknown-size Folded Spill
-  ; CHECK-NEXT:    addi sp, s0, -64
-  ; CHECK-NEXT:    lw ra, 60(sp) # 4-byte Folded Reload
-  ; CHECK-NEXT:    lw s0, 56(sp) # 4-byte Folded Reload
-  ; CHECK-NEXT:    lw s9, 52(sp) # 4-byte Folded Reload
-  ; CHECK-NEXT:    addi sp, sp, 64
+  ; CHECK-NEXT:    addi sp, s0, -80
+  ; CHECK-NEXT:    lw ra, 76(sp) # 4-byte Folded Reload
+  ; CHECK-NEXT:    lw s0, 72(sp) # 4-byte Folded Reload
+  ; CHECK-NEXT:    lw s9, 68(sp) # 4-byte Folded Reload
+  ; CHECK-NEXT:    addi sp, sp, 80
   ; CHECK-NEXT:    ret
   entry:
     ret void
   }
 
   attributes #0 = { nounwind }
 ...
 ---