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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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())
----------------
c-rhodes wrote:
> 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?
That makes sense, but let's do it as a separate patch.


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

https://reviews.llvm.org/D75672