[SLP][REVEC] Make GetMinMaxCost support FixedVectorType when REVEC is enabled.

[HanKuanChen]

No description provided.

[llvmbot]

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Han-Kuan Chen (HanKuanChen)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/114946.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (-1)
• (modified) llvm/test/Transforms/SLPVectorizer/RISCV/revec.ll (+40)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 427b8bd0e75ab0..23113b43b42c64 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -10777,7 +10777,6 @@ BoUpSLP::getEntryCost(const TreeEntry *E, ArrayRef<Value *> VectorizedVals,
     // If the selects are the only uses of the compares, they will be
     // dead and we can adjust the cost by removing their cost.
     if (VI && SelectOnly) {
-      assert(!Ty->isVectorTy() && "Expected only for scalar type.");
       auto *CI = cast<CmpInst>(VI->getOperand(0));
       IntrinsicCost -= TTI->getCmpSelInstrCost(
           CI->getOpcode(), Ty, Builder.getInt1Ty(), CI->getPredicate(),
diff --git a/llvm/test/Transforms/SLPVectorizer/RISCV/revec.ll b/llvm/test/Transforms/SLPVectorizer/RISCV/revec.ll
index 0cf4da623a0fe9..029ab4b5cb6ca3 100644
--- a/llvm/test/Transforms/SLPVectorizer/RISCV/revec.ll
+++ b/llvm/test/Transforms/SLPVectorizer/RISCV/revec.ll
@@ -94,3 +94,43 @@ entry:
   %23 = fcmp ogt <8 x float> zeroinitializer, %19
   ret void
 }
+
+define void @test3(float %0) {
+; CHECK-LABEL: @test3(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_BODY_LR_PH:%.*]]
+; CHECK:       for.body.lr.ph:
+; CHECK-NEXT:    [[TMP1:%.*]] = call <4 x float> @llvm.vector.insert.v4f32.v2f32(<4 x float> poison, <2 x float> zeroinitializer, i64 0)
+; CHECK-NEXT:    [[TMP2:%.*]] = call <4 x float> @llvm.vector.insert.v4f32.v2f32(<4 x float> [[TMP1]], <2 x float> zeroinitializer, i64 2)
+; CHECK-NEXT:    br i1 false, label [[FOR_COND_CLEANUP:%.*]], label [[FOR_BODY:%.*]]
+; CHECK:       for.cond.cleanup:
+; CHECK-NEXT:    [[TMP3:%.*]] = phi <4 x float> [ [[TMP2]], [[FOR_BODY_LR_PH]] ], [ [[TMP10:%.*]], [[FOR_BODY]] ]
+; CHECK-NEXT:    ret void
+; CHECK:       for.body:
+; CHECK-NEXT:    [[TMP4:%.*]] = load <2 x float>, ptr null, align 4
+; CHECK-NEXT:    [[TMP5:%.*]] = fcmp olt <2 x float> zeroinitializer, [[TMP4]]
+; CHECK-NEXT:    [[TMP6:%.*]] = call <4 x i1> @llvm.vector.insert.v4i1.v2i1(<4 x i1> poison, <2 x i1> <i1 true, i1 true>, i64 0)
+; CHECK-NEXT:    [[TMP7:%.*]] = call <4 x i1> @llvm.vector.insert.v4i1.v2i1(<4 x i1> [[TMP6]], <2 x i1> [[TMP5]], i64 2)
+; CHECK-NEXT:    [[TMP8:%.*]] = call <4 x float> @llvm.vector.insert.v4f32.v2f32(<4 x float> poison, <2 x float> [[TMP4]], i64 0)
+; CHECK-NEXT:    [[TMP9:%.*]] = shufflevector <4 x float> [[TMP8]], <4 x float> poison, <4 x i32> <i32 0, i32 1, i32 0, i32 1>
+; CHECK-NEXT:    [[TMP10]] = select <4 x i1> [[TMP7]], <4 x float> [[TMP9]], <4 x float> [[TMP2]]
+; CHECK-NEXT:    br label [[FOR_COND_CLEANUP]]
+;
+entry:
+  br label %for.body.lr.ph
+
+for.body.lr.ph:
+  br i1 false, label %for.cond.cleanup, label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.body, %for.body.lr.ph
+  %1 = phi <2 x float> [ zeroinitializer, %for.body.lr.ph ], [ %5, %for.body ]
+  %2 = phi <2 x float> [ zeroinitializer, %for.body.lr.ph ], [ %6, %for.body ]
+  ret void
+
+for.body:
+  %3 = load <2 x float>, ptr null, align 4
+  %4 = fcmp olt <2 x float> zeroinitializer, %3
+  %5 = select <2 x i1> <i1 true, i1 true>, <2 x float> %3, <2 x float> zeroinitializer
+  %6 = select <2 x i1> %4, <2 x float> %3, <2 x float> zeroinitializer
+  br label %for.cond.cleanup
+}

[alexey-bataev]

Shall it be something like

assert((SLPReVec || !Ty->isVectorTy()) && "Expected only for scalar type.");

?

[HanKuanChen]

I think the reason we use assert here is we don't want GetMinMaxCost accidentally remove the cost of CmpInst when the cost of vectorized code is estimated.

  case TreeEntry::MinMax: {
    auto GetScalarCost = [&](unsigned Idx) {
      return GetMinMaxCost(OrigScalarTy);
    };
    auto GetVectorCost = [&](InstructionCost CommonCost) {
      InstructionCost VecCost = GetMinMaxCost(VecTy);
      return VecCost + CommonCost;
    };
    return GetCostDiff(GetScalarCost, GetVectorCost);
  }

But GetVectorCost does not pass VI to GetMinMaxCost, so the assert will never be triggered.
Add SLPReVec here is weird because they mean different thing. It will make GetVectorCost accidentally minus the cost of CmpInst when REVEC is enabled. But it violates the original purpose of the assert.

[alexey-bataev]

Then you need to adjust the cost estimation here by changing the condition type. Also, this assertion guarantees, that it is not called for vector cost

[HanKuanChen]

Add SLPReVec is better.

[alexey-bataev]

Need to retitle PR

[dyung]

Hi @HanKuanChen, the test you modified is failing on at least one upstream bot https://lab.llvm.org/buildbot/#/builders/174/builds/8058. It might have been masked by the previous failure on the bot. Can you take a look?

[kazutakahirata]

Here is another failure:
https://lab.llvm.org/buildbot/#/builders/127/builds/1357/steps/7/logs/stdio

[kazutakahirata]

I've reverted this PR as shown above.