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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/IR/Dominators.cpp:284
   // everything in the block.
   if (isa<PHINode>(UserInst))
     return true;
----------------
kuhar wrote:
> 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.
> 
> 
Thanks for the example. That's.. weird. But, yes, I think that is allowed under the "phis execute instantly, together" interpretation. I've added a unit test that shows this.


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

https://reviews.llvm.org/D74931