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

Andrew Trick atrick at apple.com
Fri Feb 24 19:18:39 PST 2012


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



More information about the llvm-commits mailing list