[PATCH] D98169: [PoC][IR] Permit load/store/alloca for struct with the same scalable vectors.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:627
   unsigned NumElements : 31;
-  uint64_t MemberOffsets[1]; // variable sized array!
+  SmallVector<TypeSize, 8> MemberOffsets;
 
----------------
The is special code in DataLayout::getStructLayout that allocates the size of StructLayout to account for the needed size of MemberOffsets. That code does not appear to be updated in this patch.

I'm not sure we want to use SmallVector here. The minimum size is increasing the size of every StructLayout object. SmallVector contains the number of elements which is redundant with the NumElements in the StructLayout itself. SmallVector also contains an additional field for the capacity of the allocated memory.

Can we go back to the variable sized array and update DataLayout::getStructLayout to use TypeSize instead of uint64_t to calculate the needed space?


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:354
   Type *T = getMallocAllocatedType(CI, TLI);
-  if (!T || !T->isSized())
+  if (!T || !T->isSized() || T->isScalableType())
     return nullptr;
----------------
Was it already a bug that scalable vectors weren't checked for here?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:3447
+      assert(!STy->isScalableType() &&
+             "Scalable struct could not be used in GEP.");
       // For a struct, add the member offset.
----------------
"could not" -> "cannot"


================
Comment at: llvm/lib/CodeGen/Analysis.cpp:92
     // need offsets.
     const StructLayout *SL = Offsets ? DL.getStructLayout(STy) : nullptr;
     for (StructType::element_iterator EB = STy->element_begin(),
----------------
Can we remove the conditional here and always get the the struct layout? Then we don't need to check for null StructLayout in the loop. This was one of the change we did previously when we allowed scalable vectors in structs for intrinsic returns.


================
Comment at: llvm/lib/CodeGen/Analysis.cpp:111
       ComputeValueVTs(TLI, DL, EltTy, ValueVTs, MemVTs, Offsets,
-                      StartingOffset + i * EltSize);
+                      StartingOffset + EltSize * i);
     return;
----------------
Is the operand reordering required? I would hope that TypeSize multiplied by unsigned can be in either order.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CallLowering.cpp:776
     Register Addr;
+    // TODO: Review Offsets.
     MIRBuilder.materializePtrAdd(Addr, DemoteReg, OffsetLLTy, Offsets[I]);
----------------
Does this need to be addressed as part of this review?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1891
+    SmallVector<TypeSize, 4> Offsets;
+    TypeSize StartingOffset = I.getOperand(0)->getType()->isScalableType()
+                                  ? TypeSize::Scalable(0)
----------------
Maybe we need a new entrypoint to ComputeValueVTs that does this automatically based on the type passed to argument 2?


================
Comment at: llvm/lib/IR/Type.cpp:187
+
+  if (auto *STy = dyn_cast<StructType>(this)) {
+    if (STy->containsScalableVectorType())
----------------
Please address this lint warnig


================
Comment at: llvm/lib/IR/Type.cpp:188
+  if (auto *STy = dyn_cast<StructType>(this)) {
+    if (STy->containsScalableVectorType())
+      return true;
----------------
I'm slightly concerned that containsScalableVectorType() is recursive and we're now calling isScalableType in a bunch of places. This could be bad for deeply nested structs.

On that subject, are we allowing or preventing struct of scalable vectors to be part of other structs?


================
Comment at: llvm/lib/IR/Type.cpp:543
+  Type *FirstTy = getNumElements() > 0 ? elements()[0] : nullptr;
+  bool IsFirstOneScalable = false;
+  if (FirstTy)
----------------
One -> Element.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98169