[InstCombine] Fix constant swap case of fcmp + fadd + sel xfrm #119419
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
llvm:instcombine
|
@llvm/pr-subscribers-llvm-transforms Author: Rajat Bajpai (rajatbajpai) ChangesThe fcmp + fadd + sel => fcmp + sel + fadd xfrm performs incorrect transformation when select branch values are swapped. This change fixes this. Full diff: https://github.com/llvm/llvm-project/pull/119419.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index c7a0c35d099cc4..dcdb59f6e95368 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -3723,22 +3723,9 @@ static Value *foldSelectIntoAddConstant(SelectInst &SI,
if (!SIFOp || !SIFOp->hasNoSignedZeros() || !SIFOp->hasNoNaNs())
return nullptr;
- // select((fcmp Pred, X, 0), (fadd X, C), C)
- // => fadd((select (fcmp Pred, X, 0), X, 0), C)
- //
- // Pred := OGT, OGE, OLT, OLE, UGT, UGE, ULT, and ULE
- Instruction *FAdd;
- Constant *C;
- Value *X, *Z;
- CmpInst::Predicate Pred;
-
- // Note: OneUse check for `Cmp` is necessary because it makes sure that other
- // InstCombine folds don't undo this transformation and cause an infinite
- // loop. Furthermore, it could also increase the operation count.
- if (match(&SI, m_Select(m_OneUse(m_FCmp(Pred, m_Value(X), m_Value(Z))),
- m_OneUse(m_Instruction(FAdd)), m_Constant(C))) ||
- match(&SI, m_Select(m_OneUse(m_FCmp(Pred, m_Value(X), m_Value(Z))),
- m_Constant(C), m_OneUse(m_Instruction(FAdd))))) {
+ auto TryFoldIntoAddConstant =
+ [&Builder, &SI](CmpInst::Predicate Pred, Value *X, Value *Z,
+ Instruction *FAdd, Constant *C, bool Swapped) -> Value * {
// Only these relational predicates can be transformed into maxnum/minnum
// intrinsic.
if (!CmpInst::isRelational(Pred) || !match(Z, m_AnyZeroFP()))
@@ -3747,7 +3734,8 @@ static Value *foldSelectIntoAddConstant(SelectInst &SI,
if (!match(FAdd, m_FAdd(m_Specific(X), m_Specific(C))))
return nullptr;
- Value *NewSelect = Builder.CreateSelect(SI.getCondition(), X, Z, "", &SI);
+ Value *NewSelect = Builder.CreateSelect(SI.getCondition(), Swapped ? Z : X,
+ Swapped ? X : Z, "", &SI);
NewSelect->takeName(&SI);
Value *NewFAdd = Builder.CreateFAdd(NewSelect, C);
@@ -3762,7 +3750,26 @@ static Value *foldSelectIntoAddConstant(SelectInst &SI,
cast<Instruction>(NewSelect)->setFastMathFlags(NewFMF);
return NewFAdd;
- }
+ };
+
+ // select((fcmp Pred, X, 0), (fadd X, C), C)
+ // => fadd((select (fcmp Pred, X, 0), X, 0), C)
+ //
+ // Pred := OGT, OGE, OLT, OLE, UGT, UGE, ULT, and ULE
+ Instruction *FAdd;
+ Constant *C;
+ Value *X, *Z;
+ CmpInst::Predicate Pred;
+
+ // Note: OneUse check for `Cmp` is necessary because it makes sure that other
+ // InstCombine folds don't undo this transformation and cause an infinite
+ // loop. Furthermore, it could also increase the operation count.
+ if (match(&SI, m_Select(m_OneUse(m_FCmp(Pred, m_Value(X), m_Value(Z))),
+ m_OneUse(m_Instruction(FAdd)), m_Constant(C))))
+ return TryFoldIntoAddConstant(Pred, X, Z, FAdd, C, false);
+ else if (match(&SI, m_Select(m_OneUse(m_FCmp(Pred, m_Value(X), m_Value(Z))),
+ m_Constant(C), m_OneUse(m_Instruction(FAdd)))))
+ return TryFoldIntoAddConstant(Pred, X, Z, FAdd, C, true);
return nullptr;
}
diff --git a/llvm/test/Transforms/InstCombine/fcmp-fadd-select.ll b/llvm/test/Transforms/InstCombine/fcmp-fadd-select.ll
index 0d0af91608e7ac..15fad55db8df12 100644
--- a/llvm/test/Transforms/InstCombine/fcmp-fadd-select.ll
+++ b/llvm/test/Transforms/InstCombine/fcmp-fadd-select.ll
@@ -19,7 +19,7 @@ define float @test_fcmp_ogt_fadd_select_constant(float %in) {
define float @test_fcmp_ogt_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_ogt_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
-; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.maxnum.f32(float [[IN]], float 0.000000e+00)
+; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.minnum.f32(float [[IN]], float 0.000000e+00)
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
@@ -87,7 +87,7 @@ define float @test_fcmp_olt_fadd_select_constant(float %in) {
define float @test_fcmp_olt_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_olt_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
-; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.minnum.f32(float [[IN]], float 0.000000e+00)
+; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.maxnum.f32(float [[IN]], float 0.000000e+00)
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
@@ -155,7 +155,7 @@ define float @test_fcmp_oge_fadd_select_constant(float %in) {
define float @test_fcmp_oge_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_oge_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
-; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.maxnum.f32(float [[IN]], float 0.000000e+00)
+; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.minnum.f32(float [[IN]], float 0.000000e+00)
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
@@ -223,7 +223,7 @@ define float @test_fcmp_ole_fadd_select_constant(float %in) {
define float @test_fcmp_ole_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_ole_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
-; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.minnum.f32(float [[IN]], float 0.000000e+00)
+; CHECK-NEXT: [[SEL_NEW:%.*]] = call nsz float @llvm.maxnum.f32(float [[IN]], float 0.000000e+00)
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
@@ -293,7 +293,7 @@ define float @test_fcmp_ugt_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_ugt_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
; CHECK-NEXT: [[CMP1_INV:%.*]] = fcmp ole float [[IN]], 0.000000e+00
-; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float 0.000000e+00, float [[IN]]
+; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float [[IN]], float 0.000000e+00
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
@@ -366,7 +366,7 @@ define float @test_fcmp_uge_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_uge_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
; CHECK-NEXT: [[CMP1_INV:%.*]] = fcmp olt float [[IN]], 0.000000e+00
-; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float 0.000000e+00, float [[IN]]
+; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float [[IN]], float 0.000000e+00
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
@@ -439,7 +439,7 @@ define float @test_fcmp_ult_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_ult_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
; CHECK-NEXT: [[CMP1_INV:%.*]] = fcmp oge float [[IN]], 0.000000e+00
-; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float 0.000000e+00, float [[IN]]
+; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float [[IN]], float 0.000000e+00
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
@@ -512,7 +512,7 @@ define float @test_fcmp_ule_fadd_select_constant_swapped(float %in) {
; CHECK-LABEL: define float @test_fcmp_ule_fadd_select_constant_swapped(
; CHECK-SAME: float [[IN:%.*]]) {
; CHECK-NEXT: [[CMP1_INV:%.*]] = fcmp ogt float [[IN]], 0.000000e+00
-; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float 0.000000e+00, float [[IN]]
+; CHECK-NEXT: [[SEL_NEW:%.*]] = select i1 [[CMP1_INV]], float [[IN]], float 0.000000e+00
; CHECK-NEXT: [[ADD_NEW:%.*]] = fadd nnan nsz float [[SEL_NEW]], 1.000000e+00
; CHECK-NEXT: ret float [[ADD_NEW]]
;
|
|
@dtcxzyw Could you please also look at this change? |
|
Gentle ping for review @arsenm |
arsenm
approved these changes
Dec 23, 2024
The fcmp + fadd + sel => fcmp + sel + fadd xfrm performs incorrect transformation when select branch values are swapped. This change fixes this.
rajatbajpai
force-pushed
the
dev/rbajpai/fcmp-fadd-sel-issue
branch
from
c7e15d9 to
955f620
Compare
|
@dtcxzyw could you please merge this PR for me? Thank you! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/5119 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The fcmp + fadd + sel => fcmp + sel + fadd xfrm performs incorrect transformation when select branch values are swapped. This change fixes this.