[PATCH] D125787: [RISCV] Fix RVV stack frame alignment bugs

Wed May 18 09:04:37 PDT 2022

frasercrmck added a comment.

In D125787#3519315 <https://reviews.llvm.org/D125787#3519315>, @reames wrote:

> Reading through, it seems like your second bug mentioned could probably be reproduced and fixed independently of the others?  Would it make sense to split that into it's own patch?

It's likely, yeah. It's something I came across most of the way through fixing everything else so I didn't really think about it. I'm sure I could come up with a test that exercises this, and I think the fix is relatively self-contained. I'll give it a bash later.

> One bit I don't understand the reasoning on is increasing minimal RVV alignment to 16 bytes.  You say that this makes it easier to reason about, but I don't really follow that.

Yeah, it's a good question. To be honest the real reason slipped my mind while writing the description. It was in part of fixing the scalar stack alignment. Since it's underneath the RVV section and needs to be aligned to 16 bytes, unless we know that the RVV section is 16-byte aligned (in size) then we'd have to dynamically realign `sp` to keep the scalar section below it correctly aligned. This would affect the situation where our (minimum) VLEN is 64.

I thought that dynamically realigning the scalar stack was less desirable than just padding out the RVV stack size, but also that having different alignment requirements depending on the RVV version was overly complicated and just leads to maintenance burden (the simpler the FrameLowering the better, imo). Now that doesn't preclude us from generating different code to satisfy that same alignment requirement depending on our minimum VLEN, so in practice I don't expect the 16-byte alignment requirement to negatively affect us when VLEN >= 128 - once I add in that optimization.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125787