Skip to content

Conversation

@dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Jun 9, 2025

When the original predicate is ordered and both operands are non-NaN, Ordered should be set to true. This variable still matters even if both operands are non-NaN because FMF only applies to select, not fcmp.

Closes #143123.

@dtcxzyw dtcxzyw requested a review from arsenm June 9, 2025 06:06
@dtcxzyw dtcxzyw requested a review from nikic as a code owner June 9, 2025 06:06
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Jun 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 9, 2025

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

When the original predicate is ordered and both operands are non-NaN, Ordered should be set to true. If the original one is unordered, it is also safe to be converted into an ordered predicate. This variable still matters even if both operands are non-NaN because FMF only applies to select, not fcmp.

Closes #143123.


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

3 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+2)
  • (modified) llvm/test/Transforms/InstCombine/fcmp-select.ll (+16-3)
  • (modified) llvm/unittests/Analysis/ValueTrackingTest.cpp (+1-1)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index d6bb852e208f9..e65e34cda95ac 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -8660,6 +8660,8 @@ static SelectPatternResult matchSelectPattern(CmpInst::Predicate Pred,
     if (LHSSafe && RHSSafe) {
       // Both operands are known non-NaN.
       NaNBehavior = SPNB_RETURNS_ANY;
+      // It is always safe to convert the comparison to an ordered one.
+      Ordered = true;
     } else if (CmpInst::isOrdered(Pred)) {
       // An ordered comparison will return false when given a NaN, so it
       // returns the RHS.
diff --git a/llvm/test/Transforms/InstCombine/fcmp-select.ll b/llvm/test/Transforms/InstCombine/fcmp-select.ll
index 053b233cb5f04..9434f6da0b9b3 100644
--- a/llvm/test/Transforms/InstCombine/fcmp-select.ll
+++ b/llvm/test/Transforms/InstCombine/fcmp-select.ll
@@ -270,12 +270,25 @@ define i1 @test_fcmp_select_var_const_unordered(double %x, double %y) {
 }
 
 define i1 @test_fcmp_ord_select_fcmp_oeq_var_const(double %x) {
-; CHECK-LABEL:    @test_fcmp_ord_select_fcmp_oeq_var_const(
-; CHECK-NEXT:     [[CMP1:%.*]] = fcmp oeq double [[X:%.*]], 1.000000e+00
-; CHECK-NEXT:     ret i1 [[CMP1]]
+; CHECK-LABEL: @test_fcmp_ord_select_fcmp_oeq_var_const(
+; CHECK-NEXT:    [[TMP1:%.*]] = fcmp oeq double [[X:%.*]], 1.000000e+00
+; CHECK-NEXT:    ret i1 [[TMP1]]
 ;
   %cmp1 = fcmp ord double %x, 0.000000e+00
   %sel = select i1 %cmp1, double %x, double 0.000000e+00
   %cmp2 = fcmp oeq double %sel, 1.000000e+00
   ret i1 %cmp2
 }
+
+; Make sure that we recognize the SPF correctly.
+
+define float @test_select_nnan_nsz_fcmp_olt(float %x) {
+; CHECK-LABEL: @test_select_nnan_nsz_fcmp_olt(
+; CHECK-NEXT:    [[TMP1:%.*]] = fcmp olt float [[X:%.*]], -0.000000e+00
+; CHECK-NEXT:    [[SEL1:%.*]] = select i1 [[TMP1]], float [[X]], float -0.000000e+00
+; CHECK-NEXT:    ret float [[SEL1]]
+;
+  %cmp = fcmp olt float %x, 0.000000e+00
+  %sel = select nnan nsz i1 %cmp, float %x, float -0.000000e+00
+  ret float %sel
+}
diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index e23005b60891d..faf72bb17880b 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -187,7 +187,7 @@ TEST_F(MatchSelectPatternTest, FastFMin) {
       "  %A = select i1 %1, float %a, float 5.0\n"
       "  ret float %A\n"
       "}\n");
-  expectPattern({SPF_FMINNUM, SPNB_RETURNS_ANY, false});
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_ANY, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMinConstantZero) {

@dtcxzyw dtcxzyw marked this pull request as draft June 9, 2025 06:14
@dtcxzyw dtcxzyw changed the title [ValueTracking] Set Ordered to true when both operands are non-NaN. [ValueTracking] Update Ordered when both operands are non-NaN. Jun 9, 2025
@dtcxzyw dtcxzyw marked this pull request as ready for review June 9, 2025 06:30
@dtcxzyw dtcxzyw merged commit 2f15637 into llvm:main Jun 9, 2025
9 checks passed
@dtcxzyw dtcxzyw deleted the fix-143123 branch June 9, 2025 07:46
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
…m#143349)

When the original predicate is ordered and both operands are non-NaN,
`Ordered` should be set to true. This variable still matters even if
both operands are non-NaN because FMF only applies to select, not fcmp.

Closes llvm#143123.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:analysis Includes value tracking, cost tables and constant folding llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[InstCombine] fcmp is incorrectly inverted

3 participants