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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

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

> In D125787#3522637 <https://reviews.llvm.org/D125787#3522637>, @frasercrmck wrote:
>
>> In D125787#3519315 <https://reviews.llvm.org/D125787#3519315>, @reames wrote:
>>
>>> 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.
>
> Ok, I think that makes sense.  We can always chose a more complicated scheme later if we don't like the extra padding, but fixing bugs comes first.
>
> Again though, I think this is a separable patch.  Can I ask you to split that off?  We should be able to demonstrate the misaligned stack without fixing either a) internal underalignment alignments of RVV objects, or b) your offseting code (I think?).  If I understood you correctly, this basically just ends up needing to set RVVPadding more often.
>
> Hm, though one further question which maybe shows I don't fully understand.  With vl128b, is there anything preventing the actual implementation from having a VLEN=196?  My understanding was that the vlNb variants were a minimum, not an exact value.  If so, don't we need to pad out to a 16b alignment even with vl128?  (Except when actual register size is known.)

VLEN must be a power of 2 less than or equal to 65536. "The number of bits in a single vector register, VLEN ≥ ELEN, which must be a power of 2, and must be no greater than 2^16." So VLEN=196 is not possible


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125787