[PATCH] Remove "localize global" optimization

[Evan Cheng]

On Oct 3, 2013, at 3:07 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, Oct 3, 2013 at 3:00 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> 
> On Oct 2, 2013, at 11:20 AM, Alexey Samsonov <samsonov at google.com> wrote:
> 
>> 
>> On Wed, Oct 2, 2013 at 9:52 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>> 
>> On Oct 2, 2013, at 10:08 AM, Bob Wilson <bob.wilson at apple.com> wrote:
>> 
>>> 
>>> On Oct 2, 2013, at 8:33 AM, Alexey Samsonov <samsonov at google.com> wrote:
>>> 
>>>> 
>>>> On Tue, Oct 1, 2013 at 10:39 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>>>> 
>>>> On Oct 1, 2013, at 10:20 AM, Nick Lewycky <nlewycky at gmail.com> wrote:
>>>> 
>>>> >
>>>> >  The optimization was "important" for some SPEC test, but I think we're past caring about that.
>>>> 
>>>> Not necessarily.  Has anyone measured the impact on SPEC and other benchmarks?
>>>> 
>>>> I'm going to submit this anyway.
>>> 
>>> That's not very friendly.  You may not care about SPEC but that doesn't mean that no one else cares.
>>> 
>>>> See Reid's comment in http://llvm-reviews.chandlerc.com/D1754 - looks like this optimization has incorrect assumptions about C programs.
>>> 
>>> The only comment I see from Reid there is this: "I missed any earlier discussion about this. Don't we want to keep this optimization, even if it's a real corner case that fires once in a blue moon? isLeakCheckerRoot() doesn't check any flags to see if any of the sanitizers are on."
>> 
>> OK, I see the problem, and I can't think of a way to fix it.
>> 
>> But, if this hurts SPEC results, we may need to investigate alternative ways to get the same results.  Since you're the one pushing this, it would have been nice if you contributed to that effort by at least providing the performance data.  You could still do that now.  Please do.
>> 
>> Yes, the comment didn't appear in Phabricator, only the review thread:
>> 
>> "The C++ standard sez [basic.start.main]p6: "The function main shall not be used (3.2) within a program."  The C standard has no such language, so this optimization isn't really valid for C.  IIRC this caused PNaCl some problems."
>> 
>> I will run the SPEC benchmarks and measure the effects of this change. Sorry for not doing this as a first step.
> 
> It's more than just SPEC. We're seeing big regressions in Trimaran/enc-pc1, VersaBench/8b10b, among others.
> 
> And we need to be pragmatic about disabling optimizations. I feel like you are jumping to conclusion about this transformation being invalid.
> 
> ? We have an example of code that is broken by this transformation. It assumes that main is non-recursive (the comments in the transformation say as much) and that's not true in C.

That's pretty much the assumption in LLVM and the assumption for a lot of legacy code. Even though it's not necessary for the right reason, people still care about them. We need to be pragmatic about this. 

Evan

>  
>  Please revert the patch.
> 
> Evan
> 
>> 
>> -- 
>> Alexey Samsonov, MSK
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131003/79dfe6df/attachment.html>