[PATCH] Remove "localize global" optimization

[Richard Smith]

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.


>  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/3660982e/attachment.html>