-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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] Allow overflowing selects to work on commutative arguments #90812
Conversation
@llvm/pr-subscribers-llvm-transforms Author: AtariDreams (AtariDreams) ChangesThis came up when working on a patch for systemd, where the code would not simplify because the code read:
Instead of:
Full diff: https://github.com/llvm/llvm-project/pull/90812.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 8818369e79452b..d10adb1667c020 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -2075,12 +2075,27 @@ foldOverflowingAddSubSelect(SelectInst &SI, InstCombiner::BuilderTy &Builder) {
Value *FalseVal = SI.getFalseValue();
WithOverflowInst *II;
- if (!match(CondVal, m_ExtractValue<1>(m_WithOverflowInst(II))) ||
- !match(FalseVal, m_ExtractValue<0>(m_Specific(II))))
+ if (!match(CondVal, m_ExtractValue<1>(m_WithOverflowInst(II))))
return nullptr;
Value *X = II->getLHS();
Value *Y = II->getRHS();
+ if (!match(FalseVal, m_ExtractValue<0>(m_Specific(II)))) {
+ switch (II->getIntrinsicID()) {
+ case Intrinsic::uadd_with_overflow:
+ case Intrinsic::sadd_with_overflow:
+ if (!match(FalseVal, m_c_Add(m_Specific(X), m_Specific(Y))))
+ return nullptr;
+ break;
+ case Intrinsic::usub_with_overflow:
+ case Intrinsic::ssub_with_overflow:
+ if (!match(FalseVal, m_Sub(m_Specific(X), m_Specific(Y))))
+ return nullptr;
+ break;
+ default:
+ return nullptr;
+ }
+ }
auto IsSignedSaturateLimit = [&](Value *Limit, bool IsAdd) {
Type *Ty = Limit->getType();
diff --git a/llvm/test/Transforms/InstCombine/overflow_to_sat.ll b/llvm/test/Transforms/InstCombine/overflow_to_sat.ll
index 568ac77d6b88dd..eaa247b42a37b9 100644
--- a/llvm/test/Transforms/InstCombine/overflow_to_sat.ll
+++ b/llvm/test/Transforms/InstCombine/overflow_to_sat.ll
@@ -13,6 +13,18 @@ define i32 @uadd(i32 %x, i32 %y) {
ret i32 %s
}
+define i32 @uadd_cumm(i32 %x, i32 %y) {
+; CHECK-LABEL: @uadd_cumm(
+; CHECK-NEXT: [[S:%.*]] = call i32 @llvm.uadd.sat.i32(i32 [[X:%.*]], i32 [[Y:%.*]])
+; CHECK-NEXT: ret i32 [[S]]
+;
+ %ao = tail call { i32, i1 } @llvm.uadd.with.overflow.i32(i32 %x, i32 %y)
+ %o = extractvalue { i32, i1 } %ao, 1
+ %a = add i32 %y, %x
+ %s = select i1 %o, i32 -1, i32 %a
+ ret i32 %s
+}
+
define i32 @usub(i32 %x, i32 %y) {
; CHECK-LABEL: @usub(
; CHECK-NEXT: [[S:%.*]] = call i32 @llvm.usub.sat.i32(i32 [[X:%.*]], i32 [[Y:%.*]])
|
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.
This looks reasonable to me.
; | ||
%ao = tail call { i32, i1 } @llvm.uadd.with.overflow.i32(i32 %x, i32 %y) | ||
%o = extractvalue { i32, i1 } %ao, 1 | ||
%a = add i32 %y, %x |
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.
Does the case with add i32 %x, %y
here already work before this patch or do we need a test for it too?
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.
Does the case with
add i32 %x, %y
here already work before this patch or do we need a test for it too?
It already works before this patch. I tested it.
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.
Does it? https://llvm.godbolt.org/z/3Wb4ao3e3
break; | ||
case Intrinsic::ssub_with_overflow: | ||
case Intrinsic::usub_with_overflow: | ||
if (!match(FalseVal, m_Sub(m_Specific(X), m_Specific(Y)))) |
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.
Is this tested? I don't see any sub
instructions in your new tests
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.
Is this tested? I don't see any
sub
instructions in your new tests
sub already works as intended.
…w overflowing selects to work on commutative arguments This came up when working on a patch for systemd, where the code would not simplify because instead of using the actual result, someone decided to instead put "return x + y"
@AtariDreams Please never mark conversations as resolved yourself. Your resolutions are wrong half of the time. The reviewer will resolve the conversation if appropriate. |
This came up when working on a patch for systemd, where the code would not simplify because the code read:
Instead of: