-
Notifications
You must be signed in to change notification settings - Fork 11.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] Resolve TODO: Remove one-time check if other logic operand (Y) is constant #77973
Conversation
@llvm/pr-subscribers-llvm-transforms Author: AtariDreams (AtariDreams) ChangesBy using isa<Constant>(W), we do not need to worry about one-time use anymore. Full diff: https://github.com/llvm/llvm-project/pull/77973.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
index b7958978c450c9..8614aad529eb4a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -368,10 +368,14 @@ static Instruction *foldShiftOfShiftedBinOp(BinaryOperator &I,
// Find a matching one-use shift by constant. The fold is not valid if the sum
// of the shift values equals or exceeds bitwidth.
- // TODO: Remove the one-use check if the other logic operand (Y) is constant.
Value *X, *Y;
- auto matchFirstShift = [&](Value *V) {
+ auto matchFirstShift = [&](Value *V, Value *W) {
APInt Threshold(Ty->getScalarSizeInBits(), Ty->getScalarSizeInBits());
+ if (isa<Constant>(W)) {
+ return match(V, (m_BinOp(ShiftOpcode, m_Value(X), m_Constant(C0)))) &&
+ match(ConstantExpr::getAdd(C0, C1),
+ m_SpecificInt_ICMP(ICmpInst::ICMP_ULT, Threshold));
+ }
return match(V,
m_OneUse(m_BinOp(ShiftOpcode, m_Value(X), m_Constant(C0)))) &&
match(ConstantExpr::getAdd(C0, C1),
@@ -382,9 +386,9 @@ static Instruction *foldShiftOfShiftedBinOp(BinaryOperator &I,
// is not so we cannot reoder if we match operand(1) and need to keep the
// operands in their original positions.
bool FirstShiftIsOp1 = false;
- if (matchFirstShift(BinInst->getOperand(0)))
+ if (matchFirstShift(BinInst->getOperand(0), BinInst->getOperand(1)))
Y = BinInst->getOperand(1);
- else if (matchFirstShift(BinInst->getOperand(1))) {
+ else if (matchFirstShift(BinInst->getOperand(1), BinInst->getOperand(0))) {
Y = BinInst->getOperand(0);
FirstShiftIsOp1 = BinInst->getOpcode() == Instruction::Sub;
} else
|
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.
Tests?
cfc03de
to
8f9d367
Compare
67361d8
to
a9f84d5
Compare
Sure. I added a whole bunch. |
1726104
to
836c67d
Compare
15bf60c
to
ea3ee13
Compare
f68212c
to
77f7092
Compare
@nikic Alright. I think I got this right. Though I am concerned about the shl undef tests that seem to fold things into poison values. I do not know if this is what we want. |
@nikic Can I use the other tests that I removed but is affected by the transform then? |
You need the simplest possible test (no vectors...) that does not fold before your changes and folds after your changes. Please find this test on your own. |
Did. @nikic Is this good? |
@@ -433,9 +433,9 @@ define i8 @shl_sub(i8 %x, i8 %y) { | |||
define i8 @shl_sub_multiuse(i8 %x) { | |||
; CHECK-LABEL: @shl_sub_multiuse( | |||
; CHECK-NEXT: [[SH0:%.*]] = shl i8 [[X:%.*]], 3 | |||
; CHECK-NEXT: [[R:%.*]] = sub i8 [[SH0]], 42 ; constant operand on the 'sub' |
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.
Uh, did update_llc_test_checks.py really add that comment?...
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.
It removed the comment, not added it @nikic
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.
It added the comment as it was in the original IR, but when it was transformed llc removed 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.
I know, but why was it there in the first place. The previous commit added it, but that suggests the first commit is wrong.
And please do not resolve conversations off the bat like that.
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.
Because I put it there originally.
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.
Well don't then?
And for the second time, stop resolving conversations like that.
Alright @nikic Can we please merge this? |
abb1753
to
314fa05
Compare
90def1e
to
b235e03
Compare
@nikic Done! |
…tant By using match(W, m_ImmConstant()), we do not need to worry about one-time use anymore.
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
@AtariDreams We have reached out to request your feedback via the email attached to your GitHub account. Please respond to conduct@llvm.org |
By using
match(W, m_ImmConstant())
, we do not need to worry about one-time use anymore.