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

[Philip Reames via Phabricator via llvm-commits]

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:
> 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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125964