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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In D125787#3527694 <https://reviews.llvm.org/D125787#3527694>, @StephenFan wrote:

> And I also worried that if the size of RVV padding is big enough, the size of the local scalar variable field is out of the range of 12 bits signed number. If it is necessary to add an extra scavenger spill slot for it?

We already reserve a scavenger slot if there are any RVV objects at all in the frame - I believe that this one reg is sufficient to cover this eventuality. I don't think there's anything special about RVV padding compared with just the number of scalar local variables being too large for a 12-bit int in the "regular" case.



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:753
       // | scalar local variables   | | <----'
       // |--------------------------| -- <-- SP
     }
----------------
StephenFan wrote:
> If I didn't misunderstand, the SP here is assumed that align to maxalign, which is the max alignment of all stack objects (including rvv stack objects). But I found https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/MachineFrameInfo.cpp#L61 only records max align on non-scalable stack objects.
Yeah, that's right. This patch itself isn't enough - we have to update MaxAlign ourselves with the RVV objects. You probably saw that in D125973.

Although, some scalable objects //are// included in max align, as you can already see in `rvv-stack-align.mir` which does realign the stack to 32 if there's a 32-byte aligned scalable-vector alloca. I haven't dug into that to see why some are included and some aren't, but I don't think it affects our correctness once D125973 is merged.

I don't think it makes much sense to merge D125973 first as without this patch, correctly updating the max align will do nothing to fix any of the scalable misalignment issues and I don't think it'll solve any non-scalable bugs either. That's why it's based on this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125787