[PATCH] D75672: [ValueTypes] Add support for scalable EVTs

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:265
     unsigned getVectorNumElements() const {
-      assert(isVector() && "Invalid vector type!");
+      assert((isVector() || isScalableVector()) && "Invalid vector type!");
       if (isSimple())
----------------
efriedma wrote:
> c-rhodes wrote:
> > efriedma wrote:
> > > c-rhodes wrote:
> > > > efriedma wrote:
> > > > > Not sure changing getVectorNumElements like this makes sense.
> > > > I've removed this change, there was just one place in SelectionDAGBuilder I had to fix up to call `getVectorElementCount`. I had a chat with Graham and he suggested it would be good to check if it's a fixed-length vector here, maybe by having something similar to the MVT `isFixedLengthVector` on the EVT which could call `MVT::isFixedLengthVector` if it's a `SimplyTy` or check it's not a scalable vector otherwise.
> > > Yes, we want to catch incorrect use of getNumElements in both IR and in SelectionDAG.
> > Is this something we can implement going forward or should this be handled before landing this patch?
> If it's easy, might as well handle it here, if there's some non-obvious complication, I'd be fine with putting it off.
I've tested the following patch downstream:


```
diff --git a/llvm/include/llvm/CodeGen/ValueTypes.h b/llvm/include/llvm/CodeGen/ValueTypes.h
index 72e0cc8..c2db003 100644
--- a/llvm/include/llvm/CodeGen/ValueTypes.h
+++ b/llvm/include/llvm/CodeGen/ValueTypes.h
@@ -152,6 +152,10 @@ namespace llvm {
       return isSimple() ? V.isScalableVector() : isExtendedScalableVector();
     }

+    bool isFixedLengthVector() const {
+      return isSimple() ? V.isFixedLengthVector() : isExtendedFixedLengthVector();
+    }
+
     /// Return true if this is a 16-bit vector type.
     bool is16BitVector() const {
       return isSimple() ? V.is16BitVector() : isExtended16BitVector();
@@ -262,7 +266,7 @@ namespace llvm {

     /// Given a vector type, return the number of elements it contains.
     unsigned getVectorNumElements() const {
-      assert(isVector() && "Invalid vector type!");
+      assert(isFixedLengthVector() && "Invalid vector type!");
       if (isSimple())
         return V.getVectorNumElements();
       return getExtendedVectorNumElements();
@@ -432,6 +436,7 @@ namespace llvm {
     bool isExtended1024BitVector() const LLVM_READONLY;
     bool isExtended2048BitVector() const LLVM_READONLY;
     bool isExtendedScalableVector() const LLVM_READONLY;
+    bool isExtendedFixedLengthVector() const LLVM_READONLY;
     EVT getExtendedVectorElementType() const;
     unsigned getExtendedVectorNumElements() const LLVM_READONLY;
     ElementCount getExtendedVectorElementCount() const LLVM_READONLY;
diff --git a/llvm/lib/CodeGen/ValueTypes.cpp b/llvm/lib/CodeGen/ValueTypes.cpp
index dcb3a88..a2a35e2 100644
--- a/llvm/lib/CodeGen/ValueTypes.cpp
+++ b/llvm/lib/CodeGen/ValueTypes.cpp
@@ -106,6 +106,10 @@ bool EVT::isExtendedScalableVector() const {
   return isExtendedVector() && cast<VectorType>(LLVMTy)->isScalable();
 }

+bool EVT::isExtendedFixedLengthVector() const {
+  return isExtendedVector() && !cast<VectorType>(LLVMTy)->isScalable();
+}
+
 EVT EVT::getExtendedVectorElementType() const {
   assert(isExtended() && "Type is not extended!");
   return EVT::getEVT(cast<VectorType>(LLVMTy)->getElementType());
```

After running check-all there are a lot of failures. Ideally we want to get rid of `getVectorNumElements` but a quick grep shows there's >450 uses of it in lib/CodeGen and lib/Target/AArch64 alone. All of these probably aren't `EVT::getVectorNumElements` but having looked through some of the errors downstream there's cases like those in `SelectionDAGBuilder::visitShuffleVector` that need fixing up.

I think this assert is similar to the implicit cast of `TypeSize` -> `uint64_t` which Sander addressed with a warning in D75297. Being restrictive here is also going to trigger a lot of asserts, perhaps we can also emit a warning here?


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

https://reviews.llvm.org/D75672