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

[Andrew Trick]

On Feb 21, 2012, at 11:56 PM, Duncan Sands <baldrick at free.fr> wrote:

> Hi Andrew,
> 
>>   // If A is an invoke instruction, its value is only available in this normal
>>   // successor block.
>>   if (const InvokeInst *II = dyn_cast<InvokeInst>(A)) {
>>     BBA = II->getNormalDest();
>> +   if (BBA == BBB) return true;
>>   }
> 
> why is this correct?  The normal destination of an invoke could have other
> predecessors (in that case the only possible uses of the invoke value are
> in phi nodes).  For a routine of this generality, I think it is important
> to handle all cases without assuming anything about where the instructions
> in question came from (e.g. in Rafael's use case the logic could probably be
> simplified, but those special features can't be assumed here, at least not
> without auditing all uses of this method and documenting [and checking with
> assertions] the assumptions).

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:

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);
}

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.

-Andy