[PATCH] D95026: [SimplifyCFG] Update FoldBranchToCommonDest to be poison-safe

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2964
+    NewCondV = Builder.CreateBinOp(Opc, PBI->getCondition(), BICond, "or.cond");
+  Instruction *NewCond = cast<Instruction>(NewCondV);
   PBI->setCondition(NewCond);
----------------
lebedev.ri wrote:
> aqjune wrote:
> > nikic wrote:
> > > nikic wrote:
> > > > Why do we need NewCondV and NewCond? setCondition should accept a Value without the Instruction cast.
> > > Actually, one more note: What do you think about adding an impliesPoison check here? Otherwise, we may have a phase ordering issue for passes that run between SimplifyCFG and InstCombine, where one creates logical and/or and the other converts to binary. It's not a problem for folds performed by InstCombine itself, but there could be regressions for and/or folds by InstSimplify that can't happen in the meantime. Possibly this could also help LoopSimplify?
> > This is a great idea!
> > Cast is not necessary, yes. I'll fix it.
> Please add a `FIXME: remove once select form is canonical`
Safely folding select into and/or helps SCEV do further analysis: https://reviews.llvm.org/D93065#2472644 I think it might be a good idea to fold select to and/or whenever it is correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95026