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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In D91767#2410439 <https://reviews.llvm.org/D91767#2410439>, @mkazantsev wrote:

> I was considering that, but don't really see how it's useful. The existing code makes `N` iterations, for which of them it either calls `comesBefore` or `findNCD` (`findNCD` will be called as many times as many different blocks there is - 1).
>
> In the proposed solution, I'll need to:
>
> - FIll the set of blocks (because API of findNCD needs blocks)
> - Call multi-arg findNCD (which will call two-arg NCD same amount of times)
> - make comesBefore queries for all instructions in topmost block.
>
> Effectively it's always the same number of findNCD queries, overhead on set creation and potentially less number of comesBefore queries (in my solution we do it in all blocks). So overhead on set filling doesn't seem worth saving some number of cheap comesBefore queries in general case. So I just don't see why this alternative solution is better.

I was thinking more in terms of API design, not algorithmic efficiency. To me it seems like there's a clear boundary between block-level and instruction-level analysis here and I propose to separate the concerns more if this doesn't complicate the implementation.  I think that finding the NCD of multiple blocks should be useful by itself; there was even a question about it on llvm-dev fairly recently: https://groups.google.com/g/llvm-dev/c/TZ3MCpwO5Yg/m/Z4MfZz_jBAAJ.



================
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.
+  ///
----------------
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;
```


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

https://reviews.llvm.org/D91767