[PATCH] D110933: [RISCV] Add a test showing incorrect RVV stack alignment

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In D110933#3039129 <https://reviews.llvm.org/D110933#3039129>, @HsiangKai wrote:

> I didn't understand the problem. In the second test case `rvv_stack_align16`, the sp will be udpated to sp -16 - vlenb x 2. It is aligned to 16 bytes for vlenb = 8, 16, ..., etc. Isn't it?
> In addition, the memory alignment constraint is to align to the size of element. <vscale x 4 x i32> is to align 4, not align to 16.

Hi, you're right, thanks. I reduced it at the last minute a little too far. I've updated the test so that with two 8-byte objects in front it uses an offset of `24` from the stack pointer.

You're right that the minimum/desirable alignment for RVV is the same as the element size, but it's not a requirement on the maximum: just the minimum. The user can //always// supply their own alignment to something like `alloca` and we should be able to support it. And lastly, as it happens, if you use `IRBuilder` to create `alloca <vscale x 4 x i32>` you should in fact see that our `getPrefTypeAlign` does indeed return 16 despite our best wishes. I think the `DataLayout` still always aligns vectors to their (known) size.

In D110933#3038463 <https://reviews.llvm.org/D110933#3038463>, @StephenFan wrote:

> If this misalign problem does affect the correct executable of the program contain RVV stack object, and to fix it, the following two ways may make sense:
>
> 1. Deal with allocations of RVV vector object like variable sized object.
> 2. Change the RVV frame layout to what D93750 <https://reviews.llvm.org/D93750> did. Instead of calculating the total RVVStackSize and allocate them all at once, D93750 <https://reviews.llvm.org/D93750> will allocate the RVV stack objects one by one. Thus we can realign for every RVV stack object separately when it was allocating.

Thanks for the ideas. I must admit I don't know how this should be fixed, given all of the fp/sp/bp/fixed object/"rvv object"/"default object" scenarios. Given that the usual case is <= 8-byte alignment for vector objects, I wouldn't want to pessimise the default path if it can be avoided. But if we have any objects with greater alignment we should be producing the correct stack layout.

As @HsiangKai says, since VLEN is at least 64 we know any combination of vector objects is always aligned to at least 8 (I suspect this may add yet another issue when supporting VLEN=32 @craig.topper, @rogfer01). We could possibly track vector object alignment separately and realign the stack pointer if there's anything >= 8. We might need the base pointer in that scenario?

This issue is failing some of our tests though. We're creating an `alloca` and even though we can "underspecify" the alignment (from the `DataLayout`'s point of view) after creation to 4 or 8, some other LLVM passes just reset it back to the pref type align. I suppose that's legal but it was a little unexpected when I was trying to "fix" this in the IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110933