IR: handle FP predicates in CmpPredicate::getMatching

[artagnon]

CmpPredicate::getMatching implicitly assumes that both predicates are integer-predicates, and this has led to a crash being reported in VectorCombine after e409204 (VectorCombine: teach foldExtractedCmps about samesign). FP predicates are simple enough to handle as there is never any samesign information associated with them: hence handle them in CmpPredicate::getMatching, fixing the VectorCombine crash and guarding against future incorrect usages.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

CmpPredicate::getMatching implicitly assumes that both predicates are integer-predicates, and this has led to a crash being reported in VectorCombine after e409204 (VectorCombine: teach foldExtractedCmps about samesign). FP predicates are simple enough to handle as there is never any samesign information associated with them: hence handle them in CmpPredicate::getMatching, fixing the VectorCombine crash and guarding against future incorrect usages.

---

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

3 Files Affected:

• (modified) llvm/lib/IR/Instructions.cpp (+2)
• (modified) llvm/test/Transforms/VectorCombine/X86/extract-cmp-binop.ll (+19)
• (modified) llvm/unittests/IR/InstructionsTest.cpp (+1)

diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp
index b8b2c1d7f9a859..2408da410b4019 100644
--- a/llvm/lib/IR/Instructions.cpp
+++ b/llvm/lib/IR/Instructions.cpp
@@ -3939,6 +3939,8 @@ std::optional<CmpPredicate> CmpPredicate::getMatching(CmpPredicate A,
                                                       CmpPredicate B) {
   if (A.Pred == B.Pred)
     return A.HasSameSign == B.HasSameSign ? A : CmpPredicate(A.Pred);
+  if (CmpInst::isFPPredicate(A) || CmpInst::isFPPredicate(B))
+    return {};
   if (A.HasSameSign &&
       A.Pred == ICmpInst::getFlippedSignednessPredicate(B.Pred))
     return B.Pred;
diff --git a/llvm/test/Transforms/VectorCombine/X86/extract-cmp-binop.ll b/llvm/test/Transforms/VectorCombine/X86/extract-cmp-binop.ll
index 3346ebf0997f1d..2b0855dda6dc26 100644
--- a/llvm/test/Transforms/VectorCombine/X86/extract-cmp-binop.ll
+++ b/llvm/test/Transforms/VectorCombine/X86/extract-cmp-binop.ll
@@ -221,6 +221,25 @@ define i1 @different_preds(<4 x i32> %a) {
   ret i1 %r
 }
 
+; Negative test with integer and fp predicates.
+
+define i1 @different_preds_typemismtach(<4 x float> %a, <4 x i32> %b) {
+; CHECK-LABEL: @different_preds_typemismtach(
+; CHECK-NEXT:    [[E1:%.*]] = extractelement <4 x float> [[A:%.*]], i32 1
+; CHECK-NEXT:    [[E2:%.*]] = extractelement <4 x i32> [[B:%.*]], i32 2
+; CHECK-NEXT:    [[CMP1:%.*]] = fcmp ogt float [[E1]], 4.200000e+01
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp samesign ugt i32 [[E2]], -8
+; CHECK-NEXT:    [[R:%.*]] = and i1 [[CMP1]], [[CMP2]]
+; CHECK-NEXT:    ret i1 [[R]]
+;
+  %e1 = extractelement <4 x float> %a, i32 1
+  %e2 = extractelement <4 x i32> %b, i32 2
+  %cmp1 = fcmp ogt float %e1, 42.0
+  %cmp2 = icmp samesign ugt i32 %e2, -8
+  %r = and i1 %cmp1, %cmp2
+  ret i1 %r
+}
+
 ; Negative test - need 1 source vector.
 
 define i1 @different_source_vec(<4 x i32> %a, <4 x i32> %b) {
diff --git a/llvm/unittests/IR/InstructionsTest.cpp b/llvm/unittests/IR/InstructionsTest.cpp
index a46f3ca5ab5ede..b5c5510967b9fe 100644
--- a/llvm/unittests/IR/InstructionsTest.cpp
+++ b/llvm/unittests/IR/InstructionsTest.cpp
@@ -1951,6 +1951,7 @@ TEST(InstructionsTest, CmpPredicate) {
   EXPECT_EQ(*CmpPredicate::getMatching(P1, P2), CmpInst::ICMP_SLE);
   EXPECT_EQ(CmpPredicate::getMatching(P1, P2)->hasSameSign(), false);
   EXPECT_EQ(CmpPredicate::getMatching(P1, P3), std::nullopt);
+  EXPECT_EQ(CmpPredicate::getMatching(P1, CmpInst::FCMP_ULE), std::nullopt);
   EXPECT_FALSE(Q0.hasSameSign());
   EXPECT_TRUE(Q1.hasSameSign());
   EXPECT_FALSE(Q2.hasSameSign());

[nikic]

LGTM

[fhahn]

LGTM, thanks.

Looks like this fixed all related crashes I was seeing on the unreduced reproducer