[InstCombine] fold `cond ? x : -x == 0` into `x == 0` #85673

SahilPatidar · 2024-03-18T17:40:53Z

Resolve #85250
Alive2: https://alive2.llvm.org/ce/z/7DMRCy

SahilPatidar · 2024-03-18T17:42:32Z

@dtcxzyw, please check this out before making it ready for review.

SahilPatidar · 2024-03-22T15:41:38Z

@jayfoad, @arsenm

llvm/test/Transforms/InstCombine/fcmp.ll

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

llvmbot · 2024-03-22T15:57:36Z

@llvm/pr-subscribers-llvm-transforms

Author: None (SahilPatidar)

Changes

Resolve #85250
Alive2: https://alive2.llvm.org/ce/z/7DMRCy

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+7)
(modified) llvm/test/Transforms/InstCombine/fcmp.ll (+107)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 0dce0077bf1588..6d5ef3c6718554 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -7972,6 +7972,13 @@ Instruction *InstCombinerImpl::visitFCmpInst(FCmpInst &I) {
   Constant *RHSC;
   if (match(Op0, m_Instruction(LHSI)) && match(Op1, m_Constant(RHSC))) {
     switch (LHSI->getOpcode()) {
+    case Instruction::Select:
+      if ((Pred == FCmpInst::FCMP_UEQ || Pred == FCmpInst::FCMP_OEQ ||
+           Pred == FCmpInst::FCMP_UNE || Pred == FCmpInst::FCMP_ONE) &&
+          match(LHSI, m_Select(m_Value(), m_Value(X), m_FNeg(m_Value(Y)))) &&
+          X == Y && match(RHSC, m_AnyZeroFP()))
+        return new FCmpInst(Pred, X, RHSC);
+      break;
     case Instruction::PHI:
       if (Instruction *NV = foldOpIntoPhi(I, cast<PHINode>(LHSI)))
         return NV;
diff --git a/llvm/test/Transforms/InstCombine/fcmp.ll b/llvm/test/Transforms/InstCombine/fcmp.ll
index 159c84d0dd8aa9..ac5476159dd4ef 100644
--- a/llvm/test/Transforms/InstCombine/fcmp.ll
+++ b/llvm/test/Transforms/InstCombine/fcmp.ll
@@ -1284,3 +1284,110 @@ define <1 x i1> @bitcast_1vec_eq0(i32 %x) {
   %cmp = fcmp oeq <1 x float> %f, zeroinitializer
   ret <1 x i1> %cmp
 }
+
+define i1 @fcmp_ueq_sel_x_negx(float %x) {
+; CHECK-LABEL: @fcmp_ueq_sel_x_negx(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp ueq float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[RES]]
+;
+  %f = fcmp ueq float %x, 0.000000e+00
+  %neg = fneg fast float %x
+  %sel = select i1 %f, float %x, float %neg
+  %res = fcmp ueq float %sel, 0.000000e+00
+  ret i1 %res
+}
+
+define i1 @fcmp_une_sel_x_negx(float %x) {
+; CHECK-LABEL: @fcmp_une_sel_x_negx(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp une float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[RES]]
+;
+  %f = fcmp une float %x, 0.000000e+00
+  %neg = fneg fast float %x
+  %sel = select i1 %f, float %x, float %neg
+  %res = fcmp une float %sel, 0.000000e+00
+  ret i1 %res
+}
+
+define i1 @fcmp_oeq_sel_x_negx(float %x) {
+; CHECK-LABEL: @fcmp_oeq_sel_x_negx(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp oeq float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[RES]]
+;
+  %f = fcmp oeq float %x, 0.000000e+00
+  %neg = fneg fast float %x
+  %sel = select i1 %f, float %x, float %neg
+  %res = fcmp oeq float %sel, 0.000000e+00
+  ret i1 %res
+}
+
+define i1 @fcmp_one_sel_x_negx(float %x) {
+; CHECK-LABEL: @fcmp_one_sel_x_negx(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp one float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[RES]]
+;
+  %f = fcmp one float %x, 0.000000e+00
+  %neg = fneg fast float %x
+  %sel = select i1 %f, float %x, float %neg
+  %res = fcmp one float %sel, 0.000000e+00
+  ret i1 %res
+}
+
+define <8 x i1> @fcmp_ueq_sel_x_negx_vec(<8 x float> %x) {
+; CHECK-LABEL: @fcmp_ueq_sel_x_negx_vec(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp ueq <8 x float> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    ret <8 x i1> [[RES]]
+;
+  %f = fcmp ueq <8 x float> %x, zeroinitializer
+  %neg = fneg fast <8 x float> %x
+  %sel = select <8 x i1> %f, <8 x float> %x, <8 x float> %neg
+  %res = fcmp ueq <8 x float> %sel, zeroinitializer
+  ret <8 x i1> %res
+}
+
+define <8 x i1> @fcmp_une_sel_x_negx_vec(<8 x float> %x) {
+; CHECK-LABEL: @fcmp_une_sel_x_negx_vec(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp une <8 x float> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    ret <8 x i1> [[RES]]
+;
+  %f = fcmp une <8 x float> %x, zeroinitializer
+  %neg = fneg fast <8 x float> %x
+  %sel = select <8 x i1> %f, <8 x float> %x, <8 x float> %neg
+  %res = fcmp une <8 x float> %sel, zeroinitializer
+  ret <8 x i1> %res
+}
+
+define <8 x i1> @fcmp_oeq_sel_x_negx_vec(<8 x float> %x) {
+; CHECK-LABEL: @fcmp_oeq_sel_x_negx_vec(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp oeq <8 x float> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    ret <8 x i1> [[RES]]
+;
+  %f = fcmp oeq <8 x float> %x, zeroinitializer
+  %neg = fneg fast <8 x float> %x
+  %sel = select <8 x i1> %f, <8 x float> %x, <8 x float> %neg
+  %res = fcmp oeq <8 x float> %sel, zeroinitializer
+  ret <8 x i1> %res
+}
+
+define <8 x i1> @fcmp_one_sel_x_negx_vec(<8 x float> %x) {
+; CHECK-LABEL: @fcmp_one_sel_x_negx_vec(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp one <8 x float> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    ret <8 x i1> [[RES]]
+;
+  %f = fcmp one <8 x float> %x, zeroinitializer
+  %neg = fneg fast <8 x float> %x
+  %sel = select <8 x i1> %f, <8 x float> %x, <8 x float> %neg
+  %res = fcmp one <8 x float> %sel, zeroinitializer
+  ret <8 x i1> %res
+}
+
+define i1 @fcmp_sel_x_negx_with_any_cond(float %x, i1 %c) {
+; CHECK-LABEL: @fcmp_sel_x_negx_with_any_cond(
+; CHECK-NEXT:    [[RES:%.*]] = fcmp ueq float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[RES]]
+;
+  %neg = fneg float %x
+  %sel = select i1 %c, float %x, float %neg
+  %res = fcmp ueq float %sel, 0.000000e+00
+  ret i1 %res
+}

arsenm · 2024-03-22T16:01:09Z

llvm/test/Transforms/InstCombine/fcmp.ll

+  %sel = select i1 %c, float %x, float %neg
+  %res = fcmp ueq float %sel, 0.000000e+00
+  ret i1 %res
+}


Tests with -0s

And a test with denormal-fp-math=dynamic,dynamic

I'm not familiar with this what it is flag in opt.

It's an attribute, see the LangRef

I used -denormal-fp-math=dynamic in comment script, but I didn't see any difference in these tests.

I don't expect to see a difference, but alive should catch violations if it's wrong

I also think you should just directly add the attribute to a reference test or 2, not use the opt flag

I apologize for the delay. I tried adding the flag "denormal-fp-math=dynamic" after function signature but I didn't see any difference.

Correct. You should not see a difference. you should still have the test showing that it does not make a difference. Also, it should be "dynamic,dynamic" for the value

elhewaty · 2024-03-22T18:12:28Z

llvm/test/Transforms/InstCombine/fcmp.ll

Can you please split the patch into two separate commits, so it's easier to see the diff?

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

github-actions · 2024-03-23T18:55:29Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-03-23T18:55:29Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

dtcxzyw · 2024-03-25T09:54:26Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+          (match(LHSI,
+                 m_Select(m_Value(), m_Value(X), m_FNeg(m_Deferred(X)))) ||
+           match(LHSI, m_Select(m_Value(), m_FNeg(m_Value(X)), m_Deferred(X)))))
+        return new FCmpInst(Pred, X, RHSC, "", &I);


Could you please add some FMF tests?

This will drop FMF and the name of the original fcmp inst. How about:

Suggested change

return new FCmpInst(Pred, X, RHSC, "", &I);

return replaceOperand(I, 0, X);

llvm/test/Transforms/InstCombine/fcmp.ll

PR Link: llvm/llvm-project#85673

SahilPatidar · 2024-04-13T06:35:42Z

@dtcxzyw

dtcxzyw

LGTM. Please wait for additional approval from other reviewers.

llvm/test/Transforms/InstCombine/fcmp.ll

arsenm · 2024-04-17T09:45:14Z

Needs a resolve

SahilPatidar · 2024-04-19T06:39:10Z

Needs a resolve

Could you please tell me what specific issue needs to be resolved?

nikic · 2024-04-19T06:42:05Z

@SahilPatidar I think @arsenm meant rebase, not resolve :) There is a merge conflict.

Resolve llvm#85250 Alive2: https://alive2.llvm.org/ce/z/7DMRCy

dtcxzyw requested review from jayfoad, arsenm, dtcxzyw and goldsteinn March 22, 2024 15:45

dtcxzyw requested changes Mar 22, 2024

View reviewed changes

jayfoad reviewed Mar 22, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp Outdated Show resolved Hide resolved

dtcxzyw marked this pull request as ready for review March 22, 2024 15:57

dtcxzyw requested a review from nikic as a code owner March 22, 2024 15:57

llvmbot added the llvm:transforms label Mar 22, 2024

dtcxzyw requested a review from zhengyang92 March 22, 2024 15:57

arsenm reviewed Mar 22, 2024

View reviewed changes

elhewaty reviewed Mar 22, 2024

View reviewed changes

jayfoad reviewed Mar 25, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp Show resolved Hide resolved

dtcxzyw reviewed Mar 25, 2024

View reviewed changes

dtcxzyw changed the title ~~[InstCombine] FP fold, cond ? x : -x == 0 into x == 0 #85250~~ [InstCombine] fold cond ? x : -x == 0 into x == 0 Mar 25, 2024

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Mar 25, 2024

pre-commit: test PR85673

1343a62

PR Link: llvm/llvm-project#85673

dtcxzyw mentioned this pull request Mar 25, 2024

pre-commit: test PR85673 dtcxzyw/llvm-opt-benchmark#454

Closed

SahilPatidar force-pushed the resolve85250 branch from 1acdfe4 to 1d037e0 Compare April 9, 2024 09:24

SahilPatidar requested a review from dtcxzyw April 9, 2024 09:25

dtcxzyw approved these changes Apr 13, 2024

View reviewed changes

llvm/test/Transforms/InstCombine/fcmp.ll Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/fcmp.ll Outdated Show resolved Hide resolved

nikic removed their request for review April 13, 2024 11:19

jayfoad requested review from nikic and removed request for nikic April 16, 2024 16:10

arsenm approved these changes Apr 17, 2024

View reviewed changes

SahilPatidar added 6 commits April 19, 2024 12:24

[InstCombine] FP fold, cond ? x : -x == 0 into x == 0 llvm#85250

1f530c9

Fixes and updates tests

dd50785

Add and update tests for any fp zero

a5d953d

add and update fmf tests

e62038e

update test with ninf and nnan flag

785fe71

add replaceOperand

00973c3

SahilPatidar force-pushed the resolve85250 branch from 5ed882d to 00973c3 Compare April 19, 2024 06:56

arsenm merged commit 225ae82 into llvm:main Apr 20, 2024
4 checks passed

aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 21, 2024

[InstCombine] fold cond ? x : -x == 0 into x == 0 (llvm#85673)

b99ffb8

Resolve llvm#85250 Alive2: https://alive2.llvm.org/ce/z/7DMRCy

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] fold `cond ? x : -x == 0` into `x == 0` #85673

[InstCombine] fold `cond ? x : -x == 0` into `x == 0` #85673

SahilPatidar commented Mar 18, 2024 •

edited by dtcxzyw

SahilPatidar commented Mar 18, 2024

SahilPatidar commented Mar 22, 2024

llvmbot commented Mar 22, 2024

arsenm Mar 22, 2024

arsenm Mar 26, 2024

SahilPatidar Mar 26, 2024

arsenm Mar 26, 2024

SahilPatidar Mar 27, 2024

arsenm Mar 27, 2024

arsenm Mar 27, 2024

SahilPatidar Apr 8, 2024

arsenm Apr 17, 2024

elhewaty Mar 22, 2024 •

edited

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

dtcxzyw Mar 25, 2024

jayfoad Apr 16, 2024

SahilPatidar commented Apr 13, 2024

dtcxzyw left a comment

arsenm commented Apr 17, 2024

SahilPatidar commented Apr 19, 2024

nikic commented Apr 19, 2024

	return new FCmpInst(Pred, X, RHSC, "", &I);
	return replaceOperand(I, 0, X);

[InstCombine] fold cond ? x : -x == 0 into x == 0 #85673

[InstCombine] fold cond ? x : -x == 0 into x == 0 #85673

Conversation

SahilPatidar commented Mar 18, 2024 • edited by dtcxzyw

SahilPatidar commented Mar 18, 2024

SahilPatidar commented Mar 22, 2024

llvmbot commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elhewaty Mar 22, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SahilPatidar commented Apr 13, 2024

dtcxzyw left a comment

Choose a reason for hiding this comment

arsenm commented Apr 17, 2024

SahilPatidar commented Apr 19, 2024

nikic commented Apr 19, 2024

[InstCombine] fold `cond ? x : -x == 0` into `x == 0` #85673

[InstCombine] fold `cond ? x : -x == 0` into `x == 0` #85673

SahilPatidar commented Mar 18, 2024 •

edited by dtcxzyw

elhewaty Mar 22, 2024 •

edited