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

Thu May 19 09:14:00 PDT 2022

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125964