[PATCH] D74931: [Dominators] Use Instruction::comesBefore for block-local queries, NFC

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/IR/Dominators.cpp:284
   // everything in the block.
   if (isa<PHINode>(UserInst))
     return true;
----------------
vsk wrote:
> kuhar wrote:
> > This code makes every user that is a phi be dominated by the def?
> > So if we have two phis, the one that comes later may be said to dominate its predecessor?
> > 
> > Not sure if I got confused here, but this looks fishy to me. Do we need this if at all?
> Yes, that's what this is doing, and no, I'm not sure this is /really/ needed.
> 
> IIUC all phis in a block are thought to execute "instantly, together". The verifier tries to make this clear by enforcing grouping of phis at the start of a block. Perhaps this is part of the enforcement mechanism, by making `phi1 dom phi2` = `phi2 dom phi1`?
> 
> I find this inherently confusing, fwiw it's mentioned as a motivation for basic block arguments in the SIL/MLIR docs.
Ugh. But what happens when a phi has an incoming value that is another phi? Consider this pseudocode:

```llvm
; predecessors %A, %B
%a = phi i32, [0, %A], [%b, %B]
%b = phi i32  [1, %A], [%2, %B]
```

The use of `b` in `a`'s definition is considered valid from the perspective of dominance according to this code. Quickly glancing the llvm language reference I don't see anything saying you cannot use a phi value from the same basic block as an incoming value in another phi.




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

https://reviews.llvm.org/D74931