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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

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

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

I think there's something separable but I do think it'd have to bring a lot of this patch with it. To keep the base of the RVV section aligned to 16, we'd need to use this new computation of the RVV padding as a hard-coded 16 doesn't cut it. Once the padding changes, we'd also need to change some or all of the offsetting/frame index reference code to account for it.

(To clarify, with this patch the RVV padding is conceptually only the part at the //bottom// of the stack between the scalar section and the base of the RVV section. The alignment //above// the RVV section is taken into account via the `alignTo` in `getStackSizeWithRVVPadding`.

On `main` without this patch, RVV padding is more nebulous and is the combined above/below parts, so the offsetting only needs to round some offsets up to 8, rather than accurately tracking the various sections we're traversing.  For example, If the scalar section size is 8 then the RVV padding is effectively zero between the scalar and RVV sections, and is 16 above RVV. If the scalar section size is 12 then the RVV padding is 4 below and 12 above.

I personally found that quite difficult to reason about, so that's why I made the conceptual change. I tried to reflect this in the changes I made to the stack diagrams in the code - I hope it's clear enough. It's quite subtle and does affect how offsets are computed. I think with this patch the whole scheme is a bit clearer - conceptually, at least)

Anyway I guess my conclusion is that these two things are more interlinked than it may first appear, and splitting out the 16-byte stuff almost feels as though it's the majority of this patch. The smaller part may actually the ensuring of RVV alignments greater than 16 and all the internal RVV alignments as you point out.

We could also maybe split off the change that increases the //size// of the RVV stack to be a multiple of 16 bytes, while leaving it underaligned at 8. That might reduce some of the test noise involving the shifts. It might also fix the `scalar-stacka-align.ll` issue too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125787