[PATCH] D125964: [RISCV] Fix logic for determining RVV stack padding

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:948
+  const TargetRegisterInfo *TRI = STI.getRegisterInfo();
+  if (RVVStackSize && (!hasFP(MF) || TRI->hasStackRealignment(MF)) &&
+      Size % 8 != 0) {
----------------
reames wrote:
> frasercrmck wrote:
> > reames wrote:
> > > 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.)
> > 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...
> 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.  
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125964/new/

https://reviews.llvm.org/D125964