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

[Yueh-Ting (eop) Chen via Phabricator via llvm-commits]

eopXD marked 4 inline comments as done.
eopXD added inline comments.


================
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");
----------------
craig.topper wrote:
> eopXD wrote:
> > craig.topper wrote:
> > > Would StructSize be default constructed to TypeSize::Fixed(0) anyway?
> > The ctors of `TypeSize` are:
> > 
> > ```
> >   constexpr TypeSize(ScalarTy Quantity, bool Scalable)
> >       : FixedOrScalableQuantity(Quantity, Scalable) {}
> > 
> >   static constexpr TypeSize getFixed(ScalarTy ExactSize) {
> >     return TypeSize(ExactSize, false);
> >   }
> >   static constexpr TypeSize getScalable(ScalarTy MinimumSize) {
> >     return TypeSize(MinimumSize, true);
> >   }
> >   static constexpr TypeSize get(ScalarTy Quantity, bool Scalable) {
> >     return TypeSize(Quantity, Scalable);
> >   }
> >   static constexpr TypeSize Fixed(ScalarTy ExactSize) {
> >     return TypeSize(ExactSize, false);
> >   }
> >   static constexpr TypeSize Scalable(ScalarTy MinimumSize) {
> >     return TypeSize(MinimumSize, true);
> >   }
> > ```
> > 
> > Am I using it incorrectly here?
> You're right. There is no default constructor. Though it's base class has one.
Marking this thread as done.


================
Comment at: llvm/lib/IR/Type.cpp:635
       return false;
+    if (StructType *STy = dyn_cast<StructType>(Ty);
+        STy && containsHomogeneousScalableVectorTypes())
----------------
craig.topper wrote:
> Could we call isScalableTy instead of `isa<ScalableVectorType>(Ty)` above?
Yes you are correct. Corrected the code.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:767
 
+    if (ST->containsScalableVectorType())
+      return nullptr;
----------------
craig.topper wrote:
> Does SL->getSizeInBits().isScalable() below make this unnecessary?
Sorry, forgot to delete this one. Deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146872