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

[Richard Guenther]

On Thu, Sep 29, 2011 at 10:27 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> 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?

+      else if (val.lattice_val == VARYING

With default-def PARM_DECLs this should be always true, so no need
to check it.

+              && SSA_NAME_IS_DEFAULT_DEF (expr)
+              && TREE_CODE (var) == PARM_DECL
+              && POINTER_TYPE_P (TREE_TYPE (var))
+              && TYPE_USER_ALIGN (TREE_TYPE (TREE_TYPE (var))))
+       val = get_align_value (TYPE_ALIGN (TREE_TYPE (TREE_TYPE (var))),
+                              TREE_TYPE (var), 0);

I realize that the scope where this applies is pretty narrow (and thus
bad fallout is quite unlikely).  But I suppose we should document
somewhere that GCC treats alignment attributes on parameters
more strict than say, on assignments.

Btw, for this particular case Jakub invented __builtin_assume_aligned,
which I think is strictly superior.  So I'm not sure we should promote
the "old way" which isn't very well supported.  Thus, I believe frontends
should insert such builtin calls when they think it is safe to do, I
don't like the middle-end extracting too much alignment information
from types - an area that's very gray in GCC.

I'm leaving this for comments from others for now.

Thanks,
Richard.

> 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.
>
>