[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
llvm-commits at lists.llvm.org
Fri May 5 01:12:37 PDT 2023
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
More information about the llvm-commits
mailing list