[PATCH] Remove "localize global" optimization

Alexey Samsonov samsonov at google.com
Mon Oct 7 12:07:56 PDT 2013


On Mon, Oct 7, 2013 at 10:28 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.
>
>
> Were you able to do this? We need to know the full impact on the LLVM test
> suite. Ideally on more than just x86.
>
> Any change that has the potential to impact performance widely requires
> this investigation *before* the patch is committed.
>

I've reverted the change in r192121 till I'm able to do it. Once again,
sorry for the premature commit.


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


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131007/d6646c3b/attachment.html>


More information about the llvm-commits mailing list