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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked an inline comment as done.
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:
> > > > > 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.
I've created a separate patch D76376.


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

https://reviews.llvm.org/D75672