[PATCH] D91767: [DomTree][NFC] Introduce function that finds common dom of multiple instructions

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/include/llvm/IR/Dominators.h:206-207
+  ///
+  /// - If there is no dominance relation between A and B's blocks, return the
+  ///   terminator of the nearest block that dominates both of them.
+  ///
----------------
skatkov wrote:
> skatkov wrote:
> > kuhar wrote:
> > > mkazantsev wrote:
> > > > kuhar wrote:
> > > > > Does this work for invoke instructions? I'm thinking of a situation when two instructions are in a block whose immediate dominator ends with an invoke but the invoke result is not available for these two instructions.
> > > > We don't guarantee that the result will be available. We only guarantee that this invoke will be executed before we reach each of the points in arg list.
> > > I'd find it very surprising if the result of `findNCDInst` returned an instruction that didn't dominate an argument.
> > > 
> > > Note that `dominates` is defined above as:
> > > ```
> > >   /// In particular, it is worth noting that:
> > >   ///  * Non-instruction Defs dominate everything.
> > >   ///  * Def does not dominate a use in Def itself (outside of degenerate cases
> > >   ///    like unreachable code or trivial phi cycles).
> > >   ///  * Invoke/callbr Defs only dominate uses in their default destination.
> > >   bool dominates(const Value *Def, const Use &U) const;
> > > ```
> > As I understand this function is not about def and use at all while in the doc you mentioned it is about def-use chain.
> > 
> > And why do you think that result of this function does not dominate any of arguments? It does to my understanding if consider this as an extension of Basic Block domination definition.
> Does it make sense to explicitly mention in the doc function that it is not about def-use chain but about instructions and use an example with invoke to emphasize it to avoid confusion and incorrect usage of this utility function?
> As I understand this function is not about def and use at all while in the doc you mentioned it is about def-use chain.
Yeah, you are right.

> And why do you think that result of this function does not dominate any of arguments?
By 'an argument' I meant any instruction in `Insns`, not the type `Argument`. 

> It does to my understanding if consider this as an extension of Basic Block domination definition.
I got tripped of that this generalizes some existing function that checks if an instruction dominates another instruction. But now that you pointed out that the two-argument `dominates` operates on instructions and *uses*, I see that this is not the case.

Where do you plan to use `findNCDInst`? I'm trying to understand why `bool dominates(Inst, Inst)` and `Inst findNCDInst(Inst, Inst)` haven't been used before.


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

https://reviews.llvm.org/D91767