[PATCH] D82005: [InstCombine] Replace selects with Phis

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked an inline comment as done.
mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2466
+                   m_Br(m_Not(m_Specific(Cond)), m_BasicBlock(TrueSucc),
+                        m_BasicBlock(FalseSucc)))) {
+    IfTrue = Sel.getFalseValue();
----------------
nikic wrote:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > nikic wrote:
> > > > This looks like dead code, as we canonicalize branch of not by swapping successors. Might change with D81089, but for now it should be fine to just drop it.
> > > WDYM dead code? This code is needed to initialize variables `TrueSucc` and `FalseSucc`.
> > By the moment when we process this Phi, we did not process its dominator block yet. So no, this branch will not be canonicalized when we are handling this one.
> Hm, you're right. If I add an assert(0) in here, I do get some test-suite failures. D75362 should guarantee that the branch is canonicalized first, but until then it's not dead code indeed (though it should still picked up by fixpoint iteration, but best not rely on that).
It's because InstCombine iterates blocks in post-order, so dominators go after dominated blocks.


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

https://reviews.llvm.org/D82005