[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 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");
----------------
craig.topper wrote:
> 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.
Moved the assertion to check `assert(!SL->getSizeInBits().isScalable()` under `ScalarEvolution::getOffsetOfExpr`.


================
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:
> 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?


================
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);
----------------
craig.topper wrote:
> 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.
Though calling `Type::isScalableTy` still results in calling `containsScalableVectorType`, adjusted the code as requested anyway.



================
Comment at: llvm/lib/IR/Type.cpp:634
     if (!Ty->isSized(Visited))
       return false;
   }
----------------
craig.topper wrote:
> If the element is a struct containing scalable vectors, should we no longer be considered sized? That would mean we have a nested struct.
Good point. Nested structs containing scalable vector will not be consider sized. Added extra check to guard this.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146872