[PATCH] Remove "localize global" optimization
samsonov at google.com
Wed Oct 2 11:20:45 PDT 2013
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
> 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
I will run the SPEC benchmarks and measure the effects of this change.
Sorry for not doing this as a first step.
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits