[PATCH] D82072: [InstCombine] Combine select & Phi by same condition

[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:2486
     if (auto *Insn = dyn_cast<Instruction>(Inputs[Pred]))
       if (!DT.dominates(Insn, Pred->getTerminator()))
         return nullptr;
----------------
mkazantsev wrote:
> nikic wrote:
> > nikic wrote:
> > > We should add `&& Insn != Pred->getTerminator()` here to make the `@test_invoke_2_neg` test case work. What we really want to express is that `DT.dominates(Insn, Incoming)`, but as there is no `dominates(Instruction *, BasicBlockEdge)` API, this would be the closest replacement. (We could also add that API of course).
> > Actually, let's leave this to a separate patch. We should really do this via a DominatorTree API that can distinguish normal&unwind edges properly.
> This can be just `properlyDominates`.
Actually current naming in DomTree is misleading. This method is called `dominates`, but in fact it is `properlyDominates` because of this check:

```
bool DominatorTree::dominates(const Instruction *Def,
                              const Instruction *User) const {
...
  // An instruction doesn't dominate a use in itself.
  if (Def == User)
    return false;
```
So it's not a dominance check between 2 instructions, but they are also implicitly supposed to be def and user, which is counter-intuitive. I now see what you mean and agree that example with invoke should work. We need to make a global rework of this method to separate proper and usual dominance between instructions.


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

https://reviews.llvm.org/D82072