-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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] Don't use dominating conditions to transform sub into xor. #88566
Conversation
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Craig Topper (topperc) ChangesOther passes are unable to reverse this transform if we use dominating Fixes #88239. Full diff: https://github.com/llvm/llvm-project/pull/88566.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Analysis/SimplifyQuery.h b/llvm/include/llvm/Analysis/SimplifyQuery.h
index e5e6ae0d3d8e3e..a10c0dc49fa22e 100644
--- a/llvm/include/llvm/Analysis/SimplifyQuery.h
+++ b/llvm/include/llvm/Analysis/SimplifyQuery.h
@@ -113,6 +113,12 @@ struct SimplifyQuery {
using namespace PatternMatch;
return match(V, m_Undef());
}
+
+ SimplifyQuery getWithoutDomCondCache() const {
+ SimplifyQuery Copy(*this);
+ Copy.DC = nullptr;
+ return Copy;
+ }
};
} // end namespace llvm
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index 07c50d866544b3..c8734bc00c4b59 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -2281,8 +2281,10 @@ Instruction *InstCombinerImpl::visitSub(BinaryOperator &I) {
if (match(Op0, m_APInt(Op0C))) {
if (Op0C->isMask()) {
// Turn this into a xor if LHS is 2^n-1 and the remaining bits are known
- // zero.
- KnownBits RHSKnown = computeKnownBits(Op1, 0, &I);
+ // zero. We don't use information from dominating conditions so this
+ // transform is easier to reverse if necessary.
+ KnownBits RHSKnown = llvm::computeKnownBits(
+ Op1, 0, SQ.getWithInstruction(&I).getWithoutDomCondCache());
if ((*Op0C | RHSKnown.Zero).isAllOnes())
return BinaryOperator::CreateXor(Op1, Op0);
}
diff --git a/llvm/test/Transforms/InstCombine/sub-xor.ll b/llvm/test/Transforms/InstCombine/sub-xor.ll
index 71da73d51ae37e..2976598e043fee 100644
--- a/llvm/test/Transforms/InstCombine/sub-xor.ll
+++ b/llvm/test/Transforms/InstCombine/sub-xor.ll
@@ -157,3 +157,28 @@ define <2 x i8> @xor_add_splat_undef(<2 x i8> %x) {
%add = add <2 x i8> %xor, <i8 42, i8 42>
ret <2 x i8> %add
}
+
+; Make sure we don't convert sub to xor using dominating condition. That makes
+; it hard for other passe to reverse.
+define i32 @xor_dominating_cond(i32 %x) {
+; CHECK-LABEL: @xor_dominating_cond(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[COND:%.*]] = icmp ult i32 [[X:%.*]], 256
+; CHECK-NEXT: br i1 [[COND]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: [[A:%.*]] = sub nuw nsw i32 255, [[X]]
+; CHECK-NEXT: ret i32 [[A]]
+; CHECK: if.end:
+; CHECK-NEXT: ret i32 [[X]]
+;
+entry:
+ %cond = icmp ult i32 %x, 256
+ br i1 %cond, label %if.then, label %if.end
+
+if.then:
+ %a = sub i32 255, %x
+ ret i32 %a
+
+if.end:
+ ret i32 %x
+}
|
// zero. We don't use information from dominating conditions so this | ||
// transform is easier to reverse if necessary. | ||
KnownBits RHSKnown = llvm::computeKnownBits( | ||
Op1, 0, SQ.getWithInstruction(&I).getWithoutDomCondCache()); |
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 there a motivating case for when we would need/want to?
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.
Can you please add a PhaseOrdering test that demonstrates why this is necessary?
4a9dc5d
to
bba4ceb
Compare
; CHECK-NEXT: br i1 [[EXITCOND_NOT]], label [[MIDDLE_BLOCK:%.*]], label [[TMP4]], !llvm.loop [[LOOP0:![0-9]+]] | ||
; CHECK: middle.block: | ||
; CHECK-NEXT: ret void | ||
; |
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.
Okay, this seems reasonably motivating for me.
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.
LGTM
Other passes are unable to reverse this transform if we use dominating conditions. Fixes llvm#88239.
bba4ceb
to
084d6ee
Compare
Other passes are unable to reverse this transform if we use dominating
conditions.
Fixes #88239.