spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.
LGTM - thanks!
Note that we likely have FMF propagation bugs in this code based on discussion/examples in D127275 <https://reviews.llvm.org/D127275>. If you can put a TODO comment on this, that would be helpful. We'll probably need to add more tests with mixed FMF to demonstrate the bugs.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:442
// transformation we are doing here.
- if (auto *TVI = dyn_cast<BinaryOperator>(TrueVal)) {
- if (TVI->hasOneUse() && !isa<Constant>(FalseVal)) {
- if (unsigned SFO = getSelectFoldableOperands(TVI)) {
- unsigned OpToFold = 0;
- if ((SFO & 1) && FalseVal == TVI->getOperand(0)) {
- OpToFold = 1;
- } else if ((SFO & 2) && FalseVal == TVI->getOperand(1)) {
- OpToFold = 2;
- }
-
- if (OpToFold) {
- FastMathFlags FMF;
- if (isa<FPMathOperator>(&SI))
- FMF = SI.getFastMathFlags();
- Constant *C = ConstantExpr::getBinOpIdentity(
- TVI->getOpcode(), TVI->getType(), true, FMF.noSignedZeros());
- Value *OOp = TVI->getOperand(2-OpToFold);
- // Avoid creating select between 2 constants unless it's selecting
- // between 0, 1 and -1.
- const APInt *OOpC;
- bool OOpIsAPInt = match(OOp, m_APInt(OOpC));
- if (!isa<Constant>(OOp) ||
- (OOpIsAPInt && isSelect01(C->getUniqueInteger(), *OOpC))) {
- Value *NewSel = Builder.CreateSelect(SI.getCondition(), OOp, C);
+ auto FoldSelectIntoOp = [&](SelectInst &SI, Value *TrueVal, Value *FalseVal,
+ bool Swapped) -> Instruction * {
----------------
Changing only the capitalization of the enclosing function surprised me (and it doesn't seem like there's consensus on capitalization for lambda names).
"TryFoldSelectIntoOp" ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127493/new/
https://reviews.llvm.org/D127493