On 24/09/2011, at 9:40 PM, Jakub Jelinek wrote: > On Sat, Sep 24, 2011 at 11:31:25AM +0200, Richard Guenther wrote: >> In the end I'd probably say the patch is ok without the option (thus >> turned on by default), but if LC_GLOBAL_LOCALE is part of the >> glibc ABI then we clearly can't do this. > > Yes, LC_GLOBAL_LOCALE is part of glibc ABI. I guess we could only assume > the alignment if the pointer is actually dereferenced on the statement > that checks the ABI or in some stmt that dominates the spot where you want > to check the alignment. It is IMHO quite common to pass arbitrary values > in pointer types, then cast them back or just compare.
I can relate to camps of both GCC developers and GLIBC developers, and I
understand the benefits and liabilities of Tom's optimization. Ultimately, the
problem we need to solve is to find a way to manage non-conforming code with a
compiler that tries to squeeze out as much performance from valid code as
possible.
I think Tom's suggestion to have an option (either enabled or disabled by
default) is a very good solution given the circumstances. I think we should
document the option as a transitional measure designed to give time to GLIBC
and others to catch up, and obsolete it in the next release. GLIBC patch to
fix locale_t definition is attached.
In this submission Tom is being punished for his thoroughness in testing the
optimization. Let this be a lesson to all of us to steer clear of GLIBC.
[Kidding.]
We had similar discussions several times already, and it seems the guiding
principle of whether enabling new optimization that may break undefined code
patterns is to balance expected performance benefits against how frequently the
offending code pattern is used. Returning to the case at hand: Is there code
comparing a pointer to an integer? Yes. Is it common? Comparing to a zero,
absolutely; to a non-zero .... errr. Probably not that much.
The performance benefits from the optimization are quite significant. Pointer
alignment has tremendous effect on expanding __builtin_{mem,str}* functions.
Thank you,
--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics
fsf-glibc-locale_t-align.patch
Description: Binary data
