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

[Chris Lattner]

I haven't been following the whole thread, but dominators provides one specific invariant: unreachable code does not show up, so it never returns true for dominance relations.  DomTrer also had an isReachable predicate.  This system has worked quite well so far.

-Chris

On Feb 24, 2012, at 7:18 PM, Andrew Trick <atrick at apple.com> wrote:

> 
> On Feb 24, 2012, at 1:35 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> New version of the patches.
>> 
>> I have tweaked the definition of dominates(Inst, BB) to simplify the
>> implementation of dominates(Inst, Inst). Thanks a lot to Andy for
>> pointing out that the loop over the predecessors when handling a phi
>> was not necessary.
>> 
>> To test this I have reenabled the assert in
>> ScalarEvolutionExpander.cpp, bootstrapped clang and ran the
>> test-suite.
>> 
>> Cheers,
>> Rafael
>> <0001-Fix-the-implementation.patch><0002-Use-the-DT-dominate-function-in-the-verifier.patch>
> 
> LGTM!
> 
> We do need to resolve FIXMEs. I don't really understand why they occur where they do. It seems to me that dominates() has always made a fundamental assumption about reachability. For example, we've always said that two phis in the same block do not "dominate" each other. So maybe we should add a function level comment explaining that assumption. If we're not planning to change it, let's not add FIXMEs. Are there any specific cases you're concerned about?
> 
> Do you think you've tested the verifier well enough? Is there any value in running with -verify-each in this case?
> 
> -Andy
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits