[SVE] Disable INSERT_SUBVECTOR DAGCombine for scalable vectors

[cameron.mcinally]

Summary

Looking for some input on this combine wrt scalable vectors. Right now, it uses code like this:

      unsigned NumElts = VT.getVectorNumElements();
...
        if ((NumElts % Scale) == 0 && (InsIdx % Scale) == 0) {

So it's obviously not compatible with scalable vectors right now.

Has anyone thought through overloading % for ElementCount? At first glance, it seems like we can't since we don't know the vscale. Am I missing something there?

[efriedma]

Given the way INSERT_SUBVECTOR is defined, I think this transform is essentially legal if you just make NumElts an ElementCount.

ElementCount already has operator/(unsigned RHS); it would probably make sense to add a corresponding ElementCount::divisibleBy(unsigned) or something like that.

[cameron.mcinally]

Ok, I guess that makes sense. So if we're considering Min elements, should we just overload % then?

[efriedma]

Not sure we want an actual operator%. If Min is divisible by RHS, we can divide without knowing the vscale. If Min isn't divisible by RHS, though, operator/ and operator% don't make sense.

[cameron.mcinally]

I don't really have an opinion on any of this. Just thinking aloud...

If we did go with operator%, I'm not sure it's much more wrong than operator/ for scalable vector types. Assuming that the users are checking for EC % X == 0, we should be conservatively correct checking MIN % X == 0.

It would be nice if we didn't need to rewrite all the % uses to ElementCount::divisibleBy(unsigned) in existing code.

[david-arm]

Hi, there was quite a lot of discussion about the use of operators in ElementCount recently (see https://reviews.llvm.org/D86065). I think the consensus was that this needs quite a bit more thought and discussion. My patch was accepted on the basis that we'd revisit this and come up with a better plan going forward. The general problem with "/" and "%" operators is that the caller needs to be careful if the operation is lossy. I introduced the "/" operator for a specific case of dividing by 2, where the Min value was known in advance to be a power of 2. I was thinking to raise this topic at the next SVE sync call. Looking at the code above it seems like all we're asking is if one thing is a multiple of another, which could be done with a function such as isKnownMultipleOf(...), where returning true guarantees we know at compile time the answer to be true.

[cameron.mcinally]

Thanks, David. Ok, I'll put a pin in this until Thursday's call. But yeah, isKnownMultipleOf(...) sounds fair. I'll prepare a patch to see if any surprises come up.

[cameron.mcinally]

Updated with @david-arm's comments for Thursday's Sync-up call.

[efriedma]

Is there any way to write a test for the transform? Maybe not, given the limited places we use INSERT_SUBVECTOR.

The new API looks good.

[cameron.mcinally]

In D86894#2250381, @efriedma wrote:

Is there any way to write a test for the transform? Maybe not, given the limited places we use INSERT_SUBVECTOR.

There are a good number of existing tests that exercise this transform.

Also, D85364 is currently blocked by this Diff, so there are some new tests coming shortly.

[efriedma]

LGTM

llvm/include/llvm/Support/TypeSize.h

public:
 
   ElementCount operator*(unsigned RHS) {
     return { Min * RHS, Scalable };
   }
   ElementCount operator/(unsigned RHS) {
     assert(Min % RHS == 0 && "Min is not a multiple of RHS.");
     return { Min / RHS, Scalable };
   }
+  ElementCount operator%(unsigned RHS) {
+    return { Min % RHS, Scalable };
+  }
 
   bool operator==(const ElementCount& RHS) const {
     return Min == RHS.Min && Scalable == RHS.Scalable;
   }
   bool operator!=(const ElementCount& RHS) const {
     return !(*this == RHS);
   }
   bool operator==(unsigned RHS) const { return Min == RHS && !Scalable; }

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if ((N0.isUndef() || N0.getOpcode() == ISD::BITCAST) &&
     EVT N0SrcSVT = N0Src.getValueType().getScalarType();
     EVT N1SrcSVT = N1Src.getValueType().getScalarType();
     if ((N0.isUndef() || N0SrcSVT == N1SrcSVT) &&
         N0Src.getValueType().isVector() && N1Src.getValueType().isVector()) {
       EVT NewVT;
       SDLoc DL(N);
       SDValue NewIdx;
       LLVMContext &Ctx = *DAG.getContext();
-      unsigned NumElts = VT.getVectorNumElements();
+      ElementCount NumElts = VT.getVectorElementCount();
       unsigned EltSizeInBits = VT.getScalarSizeInBits();
       if ((EltSizeInBits % N1SrcSVT.getSizeInBits()) == 0) {
         unsigned Scale = EltSizeInBits / N1SrcSVT.getSizeInBits();
         NewVT = EVT::getVectorVT(Ctx, N1SrcSVT, NumElts * Scale);
         NewIdx = DAG.getVectorIdxConstant(InsIdx * Scale, DL);
       } else if ((N1SrcSVT.getSizeInBits() % EltSizeInBits) == 0) {
         unsigned Scale = N1SrcSVT.getSizeInBits() / EltSizeInBits;
         if ((NumElts % Scale) == 0 && (InsIdx % Scale) == 0) {