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

[Duncan Sands]

Hi Rafael,

>> Why assert?  The first situation occurs quite naturally if we have for
>> example
>> the CFG: entry ->  A ->  B and an instruction in B uses one in A.  Suppose the
>> edge between entry and A is deleted.  Then the IR shouldn't suddenly become
>> invalid.  Thus the instruction in A should still dominate the instruction in
>> B.  As for dominates(BBA, BBA), I don't understand why you would want it to
>> return false.  BBA doesn't properly dominate itself (properly dominates =
>> dominates and not the same basic block) of course.
>
> Because with this we have a case where dominates(BBA, BBB) =
> dominates(BBB, BBA) = true (if both are unreachable) .

yup, exactly.  It's inevitable and already occurs: in unreachable code,
instructions can use themselves for example.  Also, A can use B which
uses A etc.

  I am probably
> fine with that, but I think we have the current situation to ensure
> that dominates(A,B) != dominates(B, A).

(If A != B I guess you mean).  I don't think the property dominates(A,B) !=
dominates(B, A) is particularly useful.  In unreachable code we can have
A = bitcast B, while B = bitcast A, and it's considered valid.

   Another problem is that we
> would still have a difference on the behaviour in dominates(BBA, BBA)
> = true vs. dominates(InstA, InstA) = false.

In the instruction case it should probably be called: properly_dominates.
But I guess no one cared enough.

>
>> By the way, all the answers I gave follow directly from the definition:
>> A dominates B iff every path in the CFG from the entry block to B
>> necessarily
>> passes via A.
>
> Sure, it all depends on reading "via" as including the end point or not :-)

Yeah, that's dominates vs properly dominates.  The basic block stuff makes
this distinction already.  The instruction stuff doesn't, presumably because
only proper domination is useful; and I'm guessing it's named "dominates"
instead of "properlyDominates" for instructions because it is shorter to
write.  I don't suggest changing the name: a comment would be enough I reckon.

Ciao, Duncan.

>
>> Ciao, Duncan.
>>
>> PS: I'm happy to work on this myself, but you might need to give me a few
>> days.
>
> I started it, so I am happy to finish. We just need an agreement of
> the semantics we want. My opinions are that we should make
> dominates(BBA, BBA) false, change dominates(instA, InstA) to true or
> remane dominate(Inst, Inst) to make it clear it is not exactly the
> same thing as the basic block version.
>
> I am fine with dominates(unreachable, unreachable) returning true or asserting.
>
> Cheers,
> Rafael