[PATCH] D126465: [RISCV] Use knowledge of VLEN to avoid over-aligning the stack

Fri May 27 03:50:58 PDT 2022

frasercrmck added a comment.

In D126465#3540710 <https://reviews.llvm.org/D126465#3540710>, @reames wrote:

> In D126465#3540602 <https://reviews.llvm.org/D126465#3540602>, @craig.topper wrote:
>
>> In D126465#3540599 <https://reviews.llvm.org/D126465#3540599>, @reames wrote:
>>
>>> I'm a bit confused by this patch.  Where does the VLEN > 32 bit come from?  All I kind find in the spec is that VLEN >= ELEN, and must be a power of 2.  Given ELEN only has to be 8, shouldn't the smallest VLEN also be 8?  This seems like an utterly useless configuration, but the spec seems to allow it?
>>
>> VLEN is determined by the Zvl32b, Zvl64b, Zvl128b, etc. extensions. V implies Zvl128b. Zve64* implies Zvl64b. Zve32* implies Zvl32b. VLEN can never be less than 32 with the currently defined extensions.
>
> Thanks for the pointer.  The wording here is as follows:
> "Note: Explicit use of the Zvl32b extension string is not required for any standard vector extension as they all effectively mandate at least this minimum, but the string can be useful when stating hardware capabilities."
>
> Reviewing 18.2 and 18.3 confirms that none of the proposed vector variants allow VLEN < 32.

It's a fair point. I don't really suppose it's worth mentioning that it's at least 32 as this should work for any power of two at least 8. Do you think specifically mentioning 32 may make people assume it relies on that property?

A note on this patch I forgot to add yesterday - it does affect the nominal offsets we compute for RVV frame objects (see `wrong-stack-offset-for-rvv-object.mir` - the object is now "at" offset -8 rather than -16). Another option could be to keep the offset calculation as it is now but change how we materialize the instructions to reach those offsets. I thought this way was perhaps slightly cleaner and less error-prone: it's one place, rather than forcing all disparate users of this info to compute offsets correctly. Just it crossed my mind that someone reading `offset: -8` may be forgiven for thinking that we're not correctly aligning objects. Let me know what you think. I think both ways have their drawbacks - neither are super intuitive.

================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:896
+  // alignment. We can take this into account to avoid over-aligning the stack.
+  // Since VLEN is always a power of two greater than 32, knowing the minimum
+  // VLEN is enough to ensure the same alignment with larger VLENs.
----------------
craig.topper wrote:
> VLEN can be 32. But our calculation of vscale and scalable types is broken for it.
Ah yes, thanks - slip of the fingers. I wanted to say greater than //or equal to// 32. It's probably not worth a caveat here that we don't properly support 32 here, is it? It's largely irrelevant since the calculation works for any power of two.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126465