-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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] Check for undef first before freeze #96769
Conversation
@llvm/pr-subscribers-llvm-transforms Author: AtariDreams (AtariDreams) ChangesFull diff: https://github.com/llvm/llvm-project/pull/96769.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index c3f1c12d2f564..358b7a6b46822 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -166,7 +166,9 @@ static Value *foldMulShl1(BinaryOperator &Mul, bool CommuteOperands,
if (match(Y, m_OneUse(m_Add(m_BinOp(Shift), m_One()))) &&
match(Shift, m_OneUse(m_Shl(m_One(), m_Value(Z))))) {
bool PropagateNSW = HasNSW && Shift->hasNoSignedWrap();
- Value *FrX = Builder.CreateFreeze(X, X->getName() + ".fr");
+ Value *FrX = X;
+ if (!isGuaranteedNotToBeUndefOrPoison(X))
+ FrX = Builder.CreateFreeze(X, X->getName() + ".fr");
Value *Shl = Builder.CreateShl(FrX, Z, "mulshl", HasNUW, PropagateNSW);
return Builder.CreateAdd(Shl, FrX, Mul.getName(), HasNUW, PropagateNSW);
}
@@ -177,7 +179,9 @@ static Value *foldMulShl1(BinaryOperator &Mul, bool CommuteOperands,
// This increases uses of X, so it may require a freeze, but that is still
// expected to be an improvement because it removes the multiply.
if (match(Y, m_OneUse(m_Not(m_OneUse(m_Shl(m_AllOnes(), m_Value(Z))))))) {
- Value *FrX = Builder.CreateFreeze(X, X->getName() + ".fr");
+ Value *FrX = X;
+ if (!isGuaranteedNotToBeUndefOrPoison(X))
+ FrX = Builder.CreateFreeze(X, X->getName() + ".fr");
Value *Shl = Builder.CreateShl(FrX, Z, "mulshl");
return Builder.CreateSub(Shl, FrX, Mul.getName());
}
@@ -392,7 +396,9 @@ Instruction *InstCombinerImpl::visitMul(BinaryOperator &I) {
auto RemOpc = Div->getOpcode() == Instruction::UDiv ? Instruction::URem
: Instruction::SRem;
// X must be frozen because we are increasing its number of uses.
- Value *XFreeze = Builder.CreateFreeze(X, X->getName() + ".fr");
+ Value *XFreeze = X;
+ if (!isGuaranteedNotToBeUndefOrPoison(X))
+ XFreeze = Builder.CreateFreeze(X, X->getName() + ".fr");
Value *Rem = Builder.CreateBinOp(RemOpc, XFreeze, DivOp1);
if (DivOp1 == Y)
return BinaryOperator::CreateSub(XFreeze, Rem);
@@ -1252,7 +1258,9 @@ Instruction *InstCombinerImpl::commonIDivTransforms(BinaryOperator &I) {
// 1 / 0 --> undef ; 1 / 1 --> 1 ; 1 / -1 --> -1 ; 1 / anything else --> 0
// (Op1 + 1) u< 3 ? Op1 : 0
// Op1 must be frozen because we are increasing its number of uses.
- Value *F1 = Builder.CreateFreeze(Op1, Op1->getName() + ".fr");
+ Value *F1 = Op1;
+ if (!isGuaranteedNotToBeUndefOrPoison(Op1))
+ F1 = Builder.CreateFreeze(Op1, Op1->getName() + ".fr");
Value *Inc = Builder.CreateAdd(F1, Op0);
Value *Cmp = Builder.CreateICmpULT(Inc, ConstantInt::get(Ty, 3));
return SelectInst::Create(Cmp, F1, ConstantInt::get(Ty, 0));
@@ -2165,7 +2173,9 @@ Instruction *InstCombinerImpl::visitURem(BinaryOperator &I) {
// Op0 urem C -> Op0 < C ? Op0 : Op0 - C, where C >= signbit.
// Op0 must be frozen because we are increasing its number of uses.
if (match(Op1, m_Negative())) {
- Value *F0 = Builder.CreateFreeze(Op0, Op0->getName() + ".fr");
+ Value *F0 = Op0;
+ if (!isGuaranteedNotToBeUndefOrPoison(Op0))
+ F0 = Builder.CreateFreeze(Op0, Op0->getName() + ".fr");
Value *Cmp = Builder.CreateICmpULT(F0, Op1);
Value *Sub = Builder.CreateSub(F0, Op1);
return SelectInst::Create(Cmp, F0, Sub);
@@ -2177,7 +2187,9 @@ Instruction *InstCombinerImpl::visitURem(BinaryOperator &I) {
// urem Op0, (sext i1 X) --> (Op0 == -1) ? 0 : Op0
Value *X;
if (match(Op1, m_SExt(m_Value(X))) && X->getType()->isIntOrIntVectorTy(1)) {
- Value *FrozenOp0 = Builder.CreateFreeze(Op0, Op0->getName() + ".frozen");
+ Value *FrozenOp0 = Op0;
+ if (!isGuaranteedNotToBeUndefOrPoison(Op0))
+ FrozenOp0 = Builder.CreateFreeze(Op0, Op0->getName() + ".frozen");
Value *Cmp =
Builder.CreateICmpEQ(FrozenOp0, ConstantInt::getAllOnesValue(Ty));
return SelectInst::Create(Cmp, ConstantInt::getNullValue(Ty), FrozenOp0);
@@ -2188,7 +2200,9 @@ Instruction *InstCombinerImpl::visitURem(BinaryOperator &I) {
Value *Val =
simplifyICmpInst(ICmpInst::ICMP_ULT, X, Op1, SQ.getWithInstruction(&I));
if (Val && match(Val, m_One())) {
- Value *FrozenOp0 = Builder.CreateFreeze(Op0, Op0->getName() + ".frozen");
+ Value *FrozenOp0 = Op0;
+ if (!isGuaranteedNotToBeUndefOrPoison(Op0))
+ FrozenOp0 = Builder.CreateFreeze(Op0, Op0->getName() + ".frozen");
Value *Cmp = Builder.CreateICmpEQ(FrozenOp0, Op1);
return SelectInst::Create(Cmp, ConstantInt::getNullValue(Ty), FrozenOp0);
}
|
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.
If it's known not to be undef/poison, the freeze will be folded away, no need to check for it.
What would make sense is if you can check isGuaranteedNotToBeUndef (without the OrPoison). From a quick glance, you likely can, as these are multi-use freezes.
Fixed! |
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
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/161/builds/567 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/1272 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/1462 Here is the relevant piece of the build log for the reference:
|
All of these insert freeze due to multi-use, which is only relevant for undef values, not poison.
All of these insert freeze due to multi-use, which is only relevant for undef values, not poison.
No description provided.