[PATCH] D146872: [1/N][IR] Permit load/store/alloca for struct of the same scalable vector type

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/docs/LangRef.rst:741
 structs to facilitate intrinsics returning multiple values. Structs containing
 scalable vectors cannot be used in loads, stores, allocas, or GEPs.
 
----------------
Need to work the exception into this sentence. Right now its an absolute statement and I wouldn't expect an exception if I didn't read the next paragraph.

May add "with one exception described below" to the end of the sentence?


================
Comment at: llvm/docs/LangRef.rst:743
 
+The only exception to the above restriction is for Structs that contains
+scalable vectors of the same type (e.g. {<vscale x 2 x i32>,
----------------
Don't capitalize "Structs".


================
Comment at: llvm/docs/LangRef.rst:746
+<vscale x 2 x i32>} contains the same type while {<vscale x 2 x i32>,
+<vscale x 2 x i64>} doesn't). These kind of Structs are considered sized.
+
----------------
Don't capitalize "Structs"


================
Comment at: llvm/docs/LangRef.rst:10255
 
+Structs containing scalable vectors cannot be used in allocas. The only
+exception to the above restriction is for Structs that contains scalable
----------------
Maybe "Structs containing scalable vectors cannot be used in allocas unless all fields are the same scalable vector type"?


================
Comment at: llvm/docs/LangRef.rst:10256
+Structs containing scalable vectors cannot be used in allocas. The only
+exception to the above restriction is for Structs that contains scalable
+vectors of the same type (e.g. {<vscale x 2 x i32>, <vscale x 2 x i32>}
----------------
Same


================
Comment at: llvm/include/llvm/CodeGen/Analysis.h:75
+                     SmallVectorImpl<EVT> &ValueVTs,
+                     SmallVectorImpl<uint64_t> *FixedOffsets, uint64_t Offset);
 
----------------
Why the last field called `Offset` here but `StartingOffset` in the other two signatures?


================
Comment at: llvm/lib/CodeGen/Analysis.cpp:96
+      TypeSize EltOffset = SL ? SL->getElementOffset(EI - EB)
+                           : StartingOffset.isScalable() ? TypeSize::Scalable(0)
+                                                         : TypeSize::Fixed(0);
----------------
`TypeSize::get(0, StartingOffset.isScalable())`


================
Comment at: llvm/lib/CodeGen/Analysis.cpp:136
+  TypeSize StartingOffset =
+      Ty->isScalableTy() ? TypeSize::Scalable(Offset) : TypeSize::Fixed(Offset);
+  return ComputeValueVTs(TLI, DL, Ty, ValueVTs, Offsets, StartingOffset);
----------------
TypeSize::get(Offset, Ty->isScalableTy())


================
Comment at: llvm/lib/CodeGen/Analysis.cpp:192
   if (StructType *STy = dyn_cast<StructType>(&Ty)) {
+    assert(!STy->containsScalableVectorType() &&
+           "Unexpect scalable struct type");
----------------
This assert contradicts the comment on the next line. Scalable vectors are fine if Offsets is null


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7684
 
+  if (isa<StructType>(StoreType) &&
+      cast<StructType>(StoreType)->containsScalableVectorType())
----------------
Is this information we can get from DL.getTypeSize? Not sure when StructLayout is created and whether that would be cheaper than walking every field of a struct looking for scalable vectors.


================
Comment at: llvm/lib/IR/DataLayout.cpp:94
+  assert(!StructSize.isScalable() &&
+         "Caller of this should not be a structure with scalable type");
+  TypeSize Offset = TypeSize::Fixed(FixedOffset);
----------------
The Caller is not a structure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146872