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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov 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:
> 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?


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

https://reviews.llvm.org/D91767