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

Andrew Trick atrick at apple.com
Tue Feb 21 20:52:52 PST 2012


On Feb 20, 2012, at 9:22 PM, Rafael Ávila de Espíndola <rafael.espindola at gmail.com> wrote:

>> By the way, if you move this logic into the dominates routine, then I
>> guess the
>> verifier can be simplified by removing all the current logic and just
>> replacing
>> by a call to the improved dominator routine.
> 
> Thanks again for the review. Exactly what these functions do was a bit
> fuzzy in my head.
> 
> I really like the idea of sharing as much code as possible. The attached
> patches do that, but unfortunately they cannot fully replace the logic
> in the verifier. The interface doesn't provide which edge of a phi to
> consider, so they have to be conservative and check all.

Please remove properlyDominates(Instruction, Instruction). I created it for the same reason as your dominatesUse(). I like the idea of using dominatesUse() everywhere. Just beware that it's not semantically equivalent to the old dominates(). Can you prove to yourself that dominatesUse does what the client expects in each instance that you replaced?

I don't think anything was wrong with the original implementation except for one missing check. Can you just do this:

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

So you don't need patch #2 at all.

-Andy



More information about the llvm-commits mailing list