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

Andrew Trick atrick at apple.com
Thu Feb 23 20:26:28 PST 2012


On Feb 23, 2012, at 4:35 PM, Rafael EspĂ­ndola <rafael.espindola at gmail.com> wrote:

>> 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)".
> 
> I was missing phi in general, not just in the case of phi + invoke.
> 
> 
>> There are two BBA's above (bad style). They *both* need to dominate BBB. One of us is missing something.
> 
> Yes, they both need to dominate, but that is not sufficient. Consider
> 
>         InvokeBB
>             /\
>           /    \
>         /       X
>        /         \
>    EBB  ->   NormalBB
> 
> with the use being in NormalBB. In this case the "first BBA" is
> InvokeBB,  and it dominates NormalBB. The "second BBA" is NormalBB,
> and it dominates itself.
> 
> The BB coming from splitting the critical edge (X) doesn't dominate NormalBB.
> 
> I have attached updated versions of the patches.

Your invoke handling looks great and your comments explain it nicely (now that I parse them carefully). My initial cause for alarm was two new loops being added to a core routine. But the new loop that you've added in this case should never really be hit.

Where I got stuck before was on your new loop for phi handling, didn't read further, and got off on the invoke tangent.

If you can just remove that extra loop to handle phi operands, then it all looks good. Let me summarize.

If Use is a phi...

If Def is an invoke: apply use your existing invoke dominance logic. I don't think phis matter here.

else return properlyDominates(DefBB, PhiBB)

// A def can never dominate a phi in its own (reachable) block

// If DefBB != PhiBB, then "DefBB dominates pred(PhiBB) for all preds" is equivalent to "DefBB dominates PhiBB".

Thanks!
-Andy



More information about the llvm-commits mailing list