[PATCH] D156620: [InstCombine] Improve foldOpIntoPhi() to use isImpliedCondition

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Looks basically fine to me.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1338
+  const ICmpInst *ICmp = dyn_cast<ICmpInst>(&I);
+  if (TerminatorBI && TerminatorBI->isConditional() && ICmp) {
+    bool LHSIsTrue = TerminatorBI->getSuccessor(0) == PN->getParent();
----------------
I think this should check that getSuccessor(0) != getSuccessor(1). This will get canonicalized away, but is not guaranteed due to worklist order.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1345
+      return ConstantInt::getBool(I.getType(), ImpliedCond.value());
+  }
+
----------------
I'd personally move this block to the end, because simplifyInstructionWithOperands() is the normal case, and this is a special case.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1355
+    return NewVal;
+  }
+
----------------
Omit braces


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156620/new/

https://reviews.llvm.org/D156620