[PATCH] D70080: [AArch64][SVE] Allocate locals that are scalable vectors.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2556
 
-  // Note: We don't take allocatable stack objects into
-  // account yet, because allocation for those is not yet
-  // implemented.
+  // Create a buffer of SVE objects to allocate and sort it.
+  SmallVector<int, 8> ObjectsToAllocate;
----------------
efriedma wrote:
> sdesmalen wrote:
> > sdesmalen wrote:
> > > efriedma wrote:
> > > > cameron.mcinally wrote:
> > > > > Nit: It doesn't actually sort them, right? Or did I miss it? I'm assuming that we'll want to eventually order SVE stack objects by size.
> > > > I think we need to do something here if it's possible to have objects that aren't "16 x vscale" bytes long, to ensure natural alignment.  I'm fine leaving that as a FIXME for now, though.
> > > > Nit: It doesn't actually sort them, right? Or did I miss it? I'm assuming that we'll want to eventually order SVE stack objects by size.
> > > Yeah, I thought that made sense but realised that PEI does not seem to sort the objects either, probably because that would undo the ordering as calculated by StackSlotColoring, which orders the slots by # uses (i.e. more uses, closer to SP). I do have a separate patch that reorders the slots based on the availability of the FP, which I can share as a follow-up to this patch.
> > > I think we need to do something here if it's possible to have objects that aren't "16 x vscale" bytes long, to ensure natural alignment. I'm fine leaving that as a FIXME for now, though.
> > Objects that aren't `16 x vscale` bytes long (but have an alignment < 16bytes) are supported by this code [1]. If you're suggesting that we should reorder the objects based on natural alignment for performance reasons, we can indeed sort the ObjectsToAllocate list to group all the predicates (`vscale x 2 bytes`) together, and all the data vectors (`vscale x 16 bytes`) together (but leave their order otherwise intact to benefit from the order defined by StackSlotColoring). I can fix that in a separate patch and add a FIXME for now.
> > 
> > [1] All objects in the SVE area on the stack have the same `vscale` scaling. That means that if we have two objects, e.g. one of `vscale x 16 bytes` (data vector) and one of `vscale x 2 bytes` (predicate), this will be aligned to (`vscale *`) 16 bytes by allocating `vscale x 32 bytes` by the call to `alignTo` below on line 2578.
> Oh, I see; vscale just multiplies all the stack offsets, so anything that's aligned before vscale is applied is still aligned afterwards.  Sorry, I got confused somehow.
> 
> Sure, we can put off sorting for now.
No worries, I understand this is quite confusing. Thanks!


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

https://reviews.llvm.org/D70080