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

Jakub Kuderski via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 20 19:46:19 PST 2020


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:
> > 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.
Great, thanks for including this, especially since this isn't a part of your change.

Note that the unit test is not exectly what I described, though: in my example, the first phi was forward-referencing the second phi. I'll try to follow up on this in a separe revision this weekend of the next week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74931





More information about the llvm-commits mailing list