-
Notifications
You must be signed in to change notification settings - Fork 11.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[InstCombine] Remove m_OneUse requirement for max, but not min #81505
Conversation
@llvm/pr-subscribers-llvm-transforms Author: AtariDreams (AtariDreams) ChangesIf it is ever determined that min doesn't need one-use, then we can in fact remove that version entirely. Full diff: https://github.com/llvm/llvm-project/pull/81505.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index 56d1259e955196..4525f4e5c45832 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -2265,14 +2265,29 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
// max X, -X --> fabs X
// min X, -X --> -(fabs X)
- // TODO: Remove one-use limitation? That is obviously better for max.
- // It would be an extra instruction for min (fnabs), but that is
- // still likely better for analysis and codegen.
- if ((match(Arg0, m_OneUse(m_FNeg(m_Value(X)))) && Arg1 == X) ||
- (match(Arg1, m_OneUse(m_FNeg(m_Value(X)))) && Arg0 == X)) {
+
+ if ((match(Arg0, m_OneUse(m_FNeg(m_Value(X)))) &&
+ match(Arg1, m_Specific(X))) ||
+ (match(Arg1, m_OneUse(m_FNeg(m_Value(X)))) &&
+ match(Arg0, m_Specific(X)))) {
Value *R = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, X, II);
- if (IID == Intrinsic::minimum || IID == Intrinsic::minnum)
+ if (IID == Intrinsic::minimum || IID == Intrinsic::minnum) {
R = Builder.CreateFNegFMF(R, II);
+ }
+
+ return replaceInstUsesWith(*II, R);
+ }
+
+ // No one-use. Only for max.
+ // TODO: Remove one-use limitation? That is obviously better for max,
+ // hence why we don't check for one-use for that. However,
+ // it would be an extra instruction for min (fnabs), but
+ // that is still likely better for analysis and codegen. If so, delete
+ // one-use version
+ if ((((match(Arg0, m_FNeg(m_Value(X)))) && match(Arg1, m_Specific(X))) ||
+ (match(Arg1, m_FNeg(m_Value(X))) && match(Arg0, m_Specific(X)))) &&
+ IID != Intrinsic::minimum && IID != Intrinsic::minnum) {
+ Value *R = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, X, II);
return replaceInstUsesWith(*II, R);
}
diff --git a/llvm/test/Transforms/InstCombine/maximum.ll b/llvm/test/Transforms/InstCombine/maximum.ll
index 82e4c8794c1c84..88cb2984510002 100644
--- a/llvm/test/Transforms/InstCombine/maximum.ll
+++ b/llvm/test/Transforms/InstCombine/maximum.ll
@@ -436,7 +436,7 @@ define float @negated_op_extra_use(float %x) {
; CHECK-LABEL: @negated_op_extra_use(
; CHECK-NEXT: [[NEGX:%.*]] = fneg float [[X:%.*]]
; CHECK-NEXT: call void @use(float [[NEGX]])
-; CHECK-NEXT: [[R:%.*]] = call float @llvm.maximum.f32(float [[NEGX]], float [[X]])
+; CHECK-NEXT: [[R:%.*]] = call float @llvm.fabs.f32(float [[X]])
; CHECK-NEXT: ret float [[R]]
;
%negx = fneg float %x
diff --git a/llvm/test/Transforms/InstCombine/maxnum.ll b/llvm/test/Transforms/InstCombine/maxnum.ll
index 87288b18cbcd9f..a1a2b096cb2745 100644
--- a/llvm/test/Transforms/InstCombine/maxnum.ll
+++ b/llvm/test/Transforms/InstCombine/maxnum.ll
@@ -458,7 +458,7 @@ define float @negated_op_extra_use(float %x) {
; CHECK-LABEL: @negated_op_extra_use(
; CHECK-NEXT: [[NEGX:%.*]] = fneg float [[X:%.*]]
; CHECK-NEXT: call void @use(float [[NEGX]])
-; CHECK-NEXT: [[R:%.*]] = call float @llvm.maxnum.f32(float [[NEGX]], float [[X]])
+; CHECK-NEXT: [[R:%.*]] = call float @llvm.fabs.f32(float [[X]])
; CHECK-NEXT: ret float [[R]]
;
%negx = fneg float %x
|
|
a5ed3e1
to
1efaaf6
Compare
e9f118c
to
52b5afd
Compare
5a8264c
to
ca19acd
Compare
152d5d2
to
daeff37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not enough test diffs; the commuted cases aren't being tested
@arsenm Addressed! |
If it is ever determined that min doesn't need one-use, then we can remove the one-use requirement entirely.
@arsenm I do not have commit permissions. |
If it is ever determined that min doesn't need one-use, then we can remove the one-use requirement entirely.