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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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;
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2577
+    assert(Align <= 16 &&
+           "Alignment of scalable vectors > 16 bytes is not supported");
+    Offset = alignTo(Offset + MFI.getObjectSize(FI), Align);
----------------
sdesmalen wrote:
> efriedma wrote:
> > Assertions should only be used to check conditions we know are false.  I don't see any code that would actually guarantee this, given arbitrary IR as input.
> > 
> > Not sure what layer is best to address this.  I mean, I guess we could honor the request by turning the alloca into a dynamic alloca during isel.
> Without having thought about it too deeply, that indeed seems like a feasible way to support it. For now, are you happy for me to leave the assert here (plus an extra FIXME comment) to leave the case unsupported until we fix this in a later patch?
Please at least change it to report_fatal_error.  I'm okay putting off fixing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70080