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

Fri May 20 12:56:30 PDT 2022

reames 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) {
----------------
frasercrmck wrote:
> frasercrmck wrote:
> > 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.
> 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?
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.  

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125964