-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[InstCombine] Fix the correctness of missing check reassoc attribute #71277
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -321,6 +321,27 @@ void Instruction::setNonNeg(bool b) { | |
(b * PossiblyNonNegInst::NonNeg); | ||
} | ||
|
||
bool Instruction::hasAllowReassocOfAllOperand() const { | ||
return all_of(operands(), [](Value *V) { | ||
if (!isa<FPMathOperator>(V)) | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default should be false? |
||
|
||
auto *FPOp = cast<FPMathOperator>(V); | ||
Comment on lines
+326
to
+329
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dyn_cast instead of isa+cast |
||
switch (FPOp->getOpcode()) { | ||
case Instruction::FNeg: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't really need the opcode checks? |
||
case Instruction::FAdd: | ||
case Instruction::FSub: | ||
case Instruction::FMul: | ||
case Instruction::FDiv: | ||
case Instruction::FRem: | ||
return FPOp->hasAllowReassoc(); | ||
|
||
default: | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. default should be false? |
||
} | ||
}); | ||
} | ||
|
||
bool Instruction::hasNoUnsignedWrap() const { | ||
return cast<OverflowingBinaryOperator>(this)->hasNoUnsignedWrap(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -781,7 +781,7 @@ Instruction *InstCombinerImpl::visitFMul(BinaryOperator &I) { | |
if (Value *V = SimplifySelectsFeedingBinaryOp(I, Op0, Op1)) | ||
return replaceInstUsesWith(I, V); | ||
|
||
if (I.hasAllowReassoc()) | ||
if (I.hasAllowReassoc() && I.hasAllowReassocOfAllOperand()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a bit too specific of a query to add to Instruction. It will also be imprecise in cases where the source is a constant / load /argument or other non-instruction source |
||
if (Instruction *FoldedMul = foldFMulReassoc(I)) | ||
return FoldedMul; | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.