[PATCH] D146872: [1/11][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/lib/Analysis/ScalarEvolution.cpp:3739
     if (StructType *STy = dyn_cast<StructType>(CurTy)) {
+      assert(!STy->containsScalableVectorType() &&
+             "Structure with scalable vector type should not use GEP");
----------------
Maybe just check in `getOffsetOfExpr` where we implicitly cast getElementOffset to uint64_t? We just need to make sure the TypeSize from getElementOffset isn't scalable.


================
Comment at: llvm/lib/IR/DataLayout.cpp:49
+StructLayout::StructLayout(StructType *ST, const DataLayout &DL)
+    : StructSize(TypeSize::Fixed(0)) {
   assert(!ST->isOpaque() && "Cannot get layout of opaque structs");
----------------
Would StructSize be default constructed to TypeSize::Fixed(0) anyway?


================
Comment at: llvm/lib/IR/DataLayout.cpp:51
   assert(!ST->isOpaque() && "Cannot get layout of opaque structs");
-  StructSize = 0;
+  if (ST->containsScalableVectorType())
+    StructSize = TypeSize::Scalable(0);
----------------
Since we only support homogenous scalable structs. I wonder if we could changes StructSize to scalable on the first iteration of the below loop if the first type comes back scalable? That would save us needing to walk the entire struct hierachy separately here.

Though it gets memoized so maybe not a big deal.


================
Comment at: llvm/lib/IR/DataLayout.cpp:93
+  assert(!StructSize.isScalable() &&
+         "Object that this method acts on should not be a structure with"
+         "scalable type");
----------------
"Cannot get element at offset for structure containing scalable vector types"


================
Comment at: llvm/lib/IR/Type.cpp:634
     if (!Ty->isSized(Visited))
       return false;
   }
----------------
If the element is a struct containing scalable vectors, should we no longer be considered sized? That would mean we have a nested struct.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:740
+    // Don't unpack for structure with scalable vector.
+    if (ST->containsScalableVectorType())
+      return nullptr;
----------------
Could we check the size from the `SL` below is not scalable?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1267
 
+    // Don't unpack for structure with scalable vector.
+    if (ST->containsScalableVectorType())
----------------
Same question


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3974
 
+  if (STy->containsScalableVectorType())
+    return nullptr;
----------------
Can we check the size of the StructLayout?


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:526
+        assert(!STy->containsScalableVectorType() &&
+               "Object that this method acts on should not be a structure with"
+               "scalable type");
----------------
"GEPs are not supported on structures containing scalable vectors"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146872