[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#2410579 <https://reviews.llvm.org/D91767#2410579>, @mkazantsev wrote:

> I see the deep API problem here now. Current `dominates(Value *, Instruction *)` should not be called like that (at least because it returns `false` for `dominates(X, X)`). It has a different semantics, and should be called something like "canBeUserOf".
>
> The clear definition of dominance between two instructions is also something fishy. Specifically, it is fishy for Phi nodes from the same block (there is no precedence between them). Maybe the best name to express what we need in this patch would be `comesBefore`.
>
> It looks like a vast rework of whole DomTree API. I'm OK doing that, but do not want to have my patches waiting for this API rework (they honestly don't need). Let's unblock this branch first and then think how to name it better.

I see three ways forward:

1. Propose the renaming of the existing `.dominates` functions on llvm-dev and to that before this work.
2. Add instruction-level dominance function, but call it differently to avoid name collision, e.g., `dominatesInst`. Use this in this revision and propose refactoring afterwards.
3. Implement `findNCDInst` as a free function where it's actually necessary. Maybe this notion of dominance and NCD of instructions is domain-specific enough that there's not much value in adding this to `DominatorTree`?

I'd be most comfortable with (2) or (3).


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

https://reviews.llvm.org/D91767