[PATCH] D146872: [1/N][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/include/llvm/IR/DataLayout.h:631
+    assert(StructSize.isScalable() &&
+           "Caller of this function should be a scalable size");
+    return StructSize.getKnownMinValue();
----------------
This is a weird sentence. A "caller" is a function in the code base, it doesn't have a size.

```
"Should only be called for structs with scalable size"
```


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:289
 
+  /// Returns true if this struct contains homogeneous scalable vector types
+  bool isContainHomogeneousScalableVectorType() const;
----------------
Missing period at end of sentence


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:290
+  /// Returns true if this struct contains homogeneous scalable vector types
+  bool isContainHomogeneousScalableVectorType() const;
+
----------------
isContainHomogeneousScalableVectorType -> containsHomogenousScalableVectorTypes.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6966
                                            const SDNodeFlags Flags) {
+  if (Offset.getKnownMinValue() == 0)
+    return Base;
----------------
Why is this needed?


================
Comment at: llvm/lib/IR/DataLayout.cpp:52
+  if (ST->containsScalableVectorType())
+    StructSize = ST->containsScalableVectorType() ? TypeSize::Scalable(0)
+                                                  : TypeSize::Fixed(0);
----------------
You already checked `containsScalableVectorType` in the `if` above. You don't need to check it in the `?:` too


================
Comment at: llvm/lib/IR/DataLayout.cpp:63
     // Add padding if necessary to align the data element properly.
-    if (!isAligned(TyAlign, StructSize)) {
+    // Currently the only structure with scalable size will be the be the tuple
+    // types. Tuple types have members of the same data type so no alignment
----------------
Use "homogenous scalable vector types" here instead of "tuple types"


================
Comment at: llvm/lib/IR/Type.cpp:597
+    // This prevents it from being used in loads/stores/allocas/GEPs. The ONLY
+    // special case right now is a structure of homogenous scalable vector
+    // types.
----------------
As @nikic said. We need to return true for homogenous scalable vector structs. I put this check in when I allowed structs containing scalable vectors for intrinsic return values.  See https://reviews.llvm.org/D94142


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146872