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

[Rafael Espíndola]

> 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.

> 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.

> 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