[llvm-commits] [llvm] r150885 - /llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp

Andrew Trick atrick at apple.com
Wed Feb 22 22:20:39 PST 2012


On Feb 22, 2012, at 9:55 PM, Rafael EspĂ­ndola <rafael.espindola at gmail.com> wrote:

>> Sorry I missed that discussion.  You do additionally need to check that Invoke.BB dominates BBB. The point is that the current implementation is correct apart from invoke dominance. I don't think we want to rewrite it. I vote for:
> 
> I was also missing Phi.

I was careful to read the full thread this time. Duncan tells us thrice that the normal successor can be a critical edge. But I don't see any example of phis causing a problem. In fact when I saw the code to check phi operands, that's what prompted me to respond. It really shouldn't be necessary. Phi operand dominance is tricky with critical edges (the inverse of the invoke problem). But this interface doesn't deal with operand dominance, only instruction dominance (for all operands)--not tricky.

You can replace your "A dominates pred(phi) for all preds" check with a simple "A dominates parent(phi)".

>> if (const InvokeInst *II = dyn_cast<InvokeInst>(A)) {
>>  // Invoke cannot dominate anything in its block.
>>  if (!properlyDominates(BBA, BBB))
>>    return false;
>>  // An invoke's value is only available in its normal successor.
>>  // A critcal edge to the normal successor is ruled out by the
>>  // properlyDomintes check above.
>>  BBA = II->getNormalDest();
>>  return dominates(BBA, BBB);
>> }
> 
> This is also insufficient. BBA can dominate BBB without the "virtual
> BB" in the edge dominating BBB.

There are two BBA's above (bad style). They *both* need to dominate BBB. One of us is missing something.
-Andy

> 
>> For extra credit, you could avoid calling dominates twice by directly checking for a critical edge.
>> 
>>> By the way, I think the name dominatesUse is a bit confusing.  After all,
>>> instructions always dominates their uses, so it sounds like it should just
>>> return "true" :)  How about leaving the name "dominates" and explaining more
>>> in the comment describing the method (note that the comment in the header
>>> already doesn't match the comment here).  Or just call it:
>>> dominatesInTheAppropriateWayForThisToBeAUseOfTheOther :)
>> 
>> Agreed.
> 
> OK. I will write a new patch tomorrow with:
> 
> * Keep the name
> * Remove properlyDominates
> 
>> -Andy
> 
> Cheers,
> Rafael





More information about the llvm-commits mailing list