[PATCH] D98802: [RISCV][WIP] Fix offset computation for RVV

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:882
+  if (RVVStackSize && !hasFP(MF) && Size % 8 != 0) {
+    RVFI->setRVVPadding(getStackAlign().value());
+  }
----------------
StephenFan wrote:
> rogfer01 wrote:
> > StephenFan wrote:
> > > StephenFan wrote:
> > > > If we can set the RVVPadding as 
> > > > ```
> > > > RVFI->setRVVPadding(alignTo(Size, getStackAlign().value()) -Size);
> > > > ```
> > > > or just align to 8, because we just want the RVV stack aligned to 8 bytes.
> > > > ```
> > > > RVFI->setRVVPadding(alignTo(Size, 8) - Size);
> > > > ```
> > > > ?
> > > > In this way, maybe we can save some stack space.
> > > If we can set the RVVPadding as 
> > > ```
> > > RVFI->setRVVPadding(alignTo(Size, getStackAlign().value()) -Size);
> > > ```
> > > or just align to 8, because we just want the RVV stack aligned to 8 bytes.
> > > ```
> > > RVFI->setRVVPadding(alignTo(Size, 8) - Size);
> > > ```
> > > ?
> > > In this way, maybe we can save some stack space.
> > > If we can set the RVVPadding as
> > >
> > > RVFI->setRVVPadding(alignTo(Size, getStackAlign().value()) -Size);
> > 
> > This is more compact, I'll use that, thanks!.
> > 
> > > or just align to 8, because we just want the RVV stack aligned to 8 bytes.
> > 
> > Unless I do something different with the RVVPadding I think I need to use `getStackAlign()`.
> > 
> > Consider RV32 and a single CSR: `Size` will be 4 so `alignTo(Size, 8) - Size` is 4. `StackSize` before adding `RVVPadding` is already aligned to `getStackAlign()` (that value is commonly 16, even in RV32). If I add 4 bytes to the StackSize then I can align the RVV to 8 bytes but `sp` won't be aligned to `getStackAlign()` anymore. Maybe I'm missing something?
> You're right. I didn't consider that if the StackSize adds some value that not aligned, it will be not aligned. And then sp will not be aligned. 
Then I think the first form `RVFI->setRVVPadding(alignTo(Size, getStackAlign().value()) -Size);` would not be correct either because we might set the padding to a smaller value than `getStackAlign()`.

I'll add a clarifying comment here.


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

https://reviews.llvm.org/D98802