[PATCH] D77587: [SVE] Add new VectorType subclasses

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau marked 3 inline comments as done.
ctetreau added inline comments.


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:430
+  /// Construct a VectorType that has the same shape as some other VectorType
+  static VectorType *get(Type *ElementType, VectorType *Other) {
+    return VectorType::get(ElementType, Other->getElementCount());
----------------
efriedma wrote:
> It seems confusing to overload get() this way.
There's a bunch of code that is "I want a vector with the same shape as some other vector, but with a different element type"

Currently, there's a bunch of variants like:

```
auto *V2 = VectorType::Get(SomeTy, V1->getNumElements());
auto *V3 = VectorType::Get(SomeTy, V4->getElementType());
```

Really, for all variants of this operation, the first is potentially buggy if v1 is scalable, and it can always be replaced with the second and be correct (unless you are specifically trying to get a fixed width vector that has the same minimum number of elements as some potentially scalable vector, but that's a strange special case and I'd ask in code review that the author specifically pass false). But really, the quantity that you get out of the second argument is always noise. This change makes the following equivalent:

```
auto *V2 = VectorType::Get(SomeTy, V1->getElementType());
auto *V3 = VectorType::Get(SomeTy, V1);
assert(V2->getType() == V3->getType());
```

If V1 was scalable, then V2 is scalable, and vice versa. If you like, I can improve the documentation for this function, but I think it adds value.


================
Comment at: llvm/include/llvm/IR/Type.h:76
+    PointerTyID,       ///< 15: Pointers
+    VectorTyID,        ///< 16: SIMD 'packed' format, or other vector type
+    FixedVectorTyID,   ///< 17: Fixed width SIMD vector type
----------------
efriedma wrote:
> efriedma wrote:
> > VectorTyID should be dead.
> Oh, there's still a bunch of uses of VectorTyID.  You probably need to take care of that in this patch.
Since this thing is exposed from the C api, it might break external code to remove it.

However, it is the design that it should never be possible to construct a base VectorType. Maybe it makes sense to delete it here, but leave the type in the C api?


================
Comment at: llvm/lib/IR/Type.cpp:128
     const VectorType *VTy = cast<VectorType>(this);
-    return TypeSize(VTy->getBitWidth(), VTy->isScalable());
   }
----------------
efriedma wrote:
> I'm not sure killing off isScalable() is productive.
I'm still in the early stages of this, but if I have an instance of ScalableVectorType, then this is always true, and vice versa. It's kind of a pointless function. Usages of it tend to look like this:

```
if (isa<VectorType>(Ty) && cast<VectorType>(Ty)->isScalable())
   doStuff(Ty);
```

If it ends up being a ton of work, I'll leave it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77587