[PATCH, PR43814] Assume function arguments of pointer type are aligned.

[Tom de Vries]

On 09/29/2011 12:21 AM, Tom de Vries wrote:
> On 09/24/2011 11:31 AM, Richard Guenther wrote:
>> On Tue, Sep 20, 2011 at 1:13 PM, Tom de Vries <vries@codesourcery.com> wrote:
>>> Hi Richard,
>>>
>>> I have a patch for PR43814. It introduces an option that assumes that function
>>> arguments of pointer type are aligned, and uses that information in
>>> tree-ssa-ccp. This enables the memcpy in pr43814-2.c to be inlined.
>>>
>>> I tested the patch successfully on-by-default on x86_64 and i686 (both gcc only
>>> builds).
>>>
>>> I also tested the patch on-by-default for ARM (gcc/glibc build). The patch
>>> generated wrong code for uselocale.c:
>>> ...
>>> glibc/locale/locale.h:
>>> ...
>>> /* This value can be passed to `uselocale' and may be returned by
>>>   it. Passing this value to any other function has undefined behavior.  */
>>> # define LC_GLOBAL_LOCALE       ((__locale_t) -1L)
>>> ...
>>> glibc/locale/uselocale.c:
>>> ...
>>> locale_t
>>> __uselocale (locale_t newloc)
>>> {
>>>  locale_t oldloc = _NL_CURRENT_LOCALE;
>>>
>>>  if (newloc != NULL)
>>>    {
>>>      const locale_t locobj
>>>        = newloc == LC_GLOBAL_LOCALE ? &_nl_global_locale : newloc;
>>>
>>> ...
>>> The assumption that function arguments of pointer type are aligned, allowed the
>>> test 'newloc == LC_GLOBAL_LOCALE' to evaluate to false.
>>> But the usage of ((__locale_t) -1L) as function argument in uselocale violates
>>> that assumption.
>>>
>>> Fixing the definition of LC_GLOBAL_LOCALE allowed the gcc tests to run without
>>> regressions for ARM.
>>>
>>> Furthermore, the patch fixes ipa-sra-2.c and ipa-sra-6.c regressions on ARM,
>>> discussed here:
>>> - http://gcc.gnu.org/ml/gcc-patches/2011-08/msg00930.html
>>> - http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00459.html
>>>
>>> But, since glibc uses this construct currently, the option is off-by-default for
>>> now.
>>>
>>> OK for trunk?
>>
>> No, I don't like to have an option to control this.  And given the issue
>> you spotted it doesn't look like the best idea either.  This area in GCC
>> is simply too fragile right now :/
>>
>> It would be nice if we could extend IPA-CP to propagate alignment
>> information though.
>>
>> And at some point devise a reliable way for frontends to communicate
>> alignment constraints the middle-end can rely on (well, yes, you could
>> argue that's what TYPE_ALIGN is about, and I thought that maybe we
>> can unconditionally rely on TYPE_ALIGN for pointer PARM_DECLs
>> at least - your example shows we can't).
>>
>> 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.
>>
> 
> I removed the flag, and enabled the optimization now with the aligned attribute.
> 
> bootstrapped and tested on x86_64 (gcc only build), and build and reg-tested on
> arm (gcc + glibc build), no issues found.
> 

Sorry for the EWRONGPATCH. Correct patch this time.

OK for trunk?

Thanks,
- Tom

> 
> I intend to send a follow-up patch that introduces a target hook
> function_pointers_aligned, that is false by default, and on by default for
> -mandroid. I asked Google to test their Android codebase with the optimization
> on by default. Would such a target hook be acceptable?
> 
>> Richard.
>>
> 
> Thanks,
> - Tom
> 
> 2011-09-29  Tom de Vries <tom@codesourcery.com>
> 
> 	PR target/43814
> 	* tree-ssa-ccp.c (get_align_value): New function, factored out of
> 	get_value_from_alignment.
> 	(get_value_from_alignment): Use get_align_value.
> 	(get_value_for_expr): Use get_align_value to handle alignment of
> 	function argument pointers with TYPE_USER_ALIGN.
> 
> 	* gcc/testsuite/gcc.dg/pr43814.c: New test.
> 	* gcc/testsuite/gcc.target/arm/pr43814-2.c: New test.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr43814.5.patch
Type: text/x-patch
Size: 4100 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20110929/f21b4f41/attachment.bin>