[PATCH] Remove "localize global" optimization

[Nick Lewycky]

On 3 October 2013 15:00, 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.
>

For enc-pc1, you can see global variables "c1" and "count" are only used in
main(), while "ax", "bx", "cx", and "dx" are only used in assemble(). I
haven't looked at 8b10b.

And we need to be pragmatic about disabling optimizations. I feel like you
> are jumping to conclusion about this transformation being invalid.
>

The transformation *is* invalid, it is jumping to the conclusion that it
can move globals to allocas in main with no
evidence/proof/attempt-to-determine whether main is reentrant (or for that
matter, the entry point at all, though the latter is assumed all over llvm).

Nick

 Please revert the patch.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131003/597a28f5/attachment.html>