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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetFrameLowering.h:369
+  /// support multiple StackIDs may want to override this method.
+  virtual TargetStackID::Value getStackIDForType(const Type *T) const {
+    return TargetStackID::Default;
----------------
We don't want to encourage people to key backend behavior off of the IR type of an alloca.  Please change the API so it only passes down whether the type has a variable size.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70080