[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

ElementCount &operator/=(unsigned RHS) {
     Min /= RHS;
     return *this;
   }
 
   ElementCount NextPowerOf2() const {
     return {(unsigned)llvm::NextPowerOf2(Min), Scalable};
   }
 
+  bool isKnownMultipleOf(unsigned RHS) const {
+    return Min % RHS == 0;
+  }
+
   static ElementCount getFixed(unsigned Min) { return {Min, false}; }
   static ElementCount getScalable(unsigned Min) { return {Min, true}; }
   static ElementCount get(unsigned Min, bool Scalable) {
     return {Min, Scalable};
   }
 
   /// Printing function.
   void print(raw_ostream &OS) const {

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) {
+        if (NumElts.isKnownMultipleOf(Scale) && (InsIdx % Scale) == 0) {
           NewVT = EVT::getVectorVT(Ctx, N1SrcSVT, NumElts / Scale);
           NewIdx = DAG.getVectorIdxConstant(InsIdx / Scale, DL);
         }
       }
       if (NewIdx && hasOperation(ISD::INSERT_SUBVECTOR, NewVT)) {
         SDValue Res = DAG.getBitcast(NewVT, N0Src);
         Res = DAG.getNode(ISD::INSERT_SUBVECTOR, DL, NewVT, Res, N1Src, NewIdx);
         return DAG.getBitcast(VT, Res);