[llvm-commits] [llvm] r151466 - in /llvm/trunk: include/llvm/Analysis/Dominators.h lib/Analysis/InstructionSimplify.cpp

[Rafael Espíndola]

> The original dominates(Inst, Inst) did not implement proper dominance. I
> added a properlyDominates(Inst, Inst) for that purpose. Rafael later
> reasoned that all clients of dominates(Inst, Inst) actually want proper
> dominance and changed the implementation accordingly--I tend to agree
> because I've seen multiple bugs based on that assumption. I suggested
> removing properlyDominates(Inst, Inst), because it's misleading if it
> actually does the same thing as dominates(Inst, Inst). Given that it only
> makes sense to implement instruction-level queries as proper dominance (def
> dominates use is the implied semantics), it shouldn't matter what the name
> is, as long as the comments are clear.

Thanks for the comments! I think the summary of the thread so far is

* Lets keep the current names, just make it clear that dominates(inst,
inst) is not exactly the same as dominates(bb,bb).
* Dominates for instructions stays as is.
* Dominates for BB is changed so that anything dominates an
unreachable block. This does create a case where
  A != B && dominates(A, B) && dominates(B,A) is true.

I still have a small preference for renaming one of the functions or
making them behave more like one another, but I agree that this would
be a bit better than what we have now. Chris, would you be OK with a
patch implementing it?

> -Andy

Cheers,
Rafael