diff mbox

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

Message ID 4E842BD6.9030102@mentor.com
State New
Headers show

Commit Message

Tom de Vries Sept. 29, 2011, 8:27 a.m. UTC 
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.

Comments

Richard Biener Sept. 29, 2011, 9:23 a.m. UTC | #1 
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.
>
>
Maxim Kuvyrkov Oct. 18, 2011, 11:58 p.m. UTC | #2 
On 29/09/2011, at 10:23 PM, Richard Guenther wrote:

> On Thu, Sep 29, 2011 at 10:27 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:

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

Richard, the intention here is for a follow up patch to change "&& TYPE_USER_ALIGN" to "&& (TYPE_USER_ALIGN || flag_assume_aligned_parameters)".  I understand that you will probably not like the idea of adding flag_assume_aligned_parameters, but it wouldn't make such an option any less valuable for experienced users of GCC.  For some users this option will the additional piece of rope to hang themselves; for others it will produce good benefits of better performance.

[Disclaimer: I've put significant amount of my time into investigation of this problem and hate to see it all go to waste :-).]

Thank you,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics
Richard Biener Oct. 19, 2011, 8:43 a.m. UTC | #3 
On Wed, Oct 19, 2011 at 1:58 AM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote:
> On 29/09/2011, at 10:23 PM, Richard Guenther wrote:
>
>> On Thu, Sep 29, 2011 at 10:27 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>
>> +              && 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.
>
> Richard, the intention here is for a follow up patch to change "&& TYPE_USER_ALIGN" to "&& (TYPE_USER_ALIGN || flag_assume_aligned_parameters)".  I understand that you will probably not like the idea of adding flag_assume_aligned_parameters, but it wouldn't make such an option any less valuable for experienced users of GCC.  For some users this option will the additional piece of rope to hang themselves; for others it will produce good benefits of better performance.
>
> [Disclaimer: I've put significant amount of my time into investigation of this problem and hate to see it all go to waste :-).]

It's not wasted, but using it as part of a bad solution isn't good either ;)

I do want a start to a more general solution that also will fix the ever present
wrong-code bugs on strict-align targets with respect to excessive alignment
we assume for them.  This doesn't seem to be one, but instead it seems
to introduce more inconsistencies which is very bad.

What exact problems do you try to solve (I presume not the artificial
testcase in the PR)?

Thanks,
Richard.
diff mbox

Patch

Index: gcc/tree-ssa-ccp.c
===================================================================
--- gcc/tree-ssa-ccp.c (revision 179210)
+++ gcc/tree-ssa-ccp.c (working copy)
@@ -500,20 +500,14 @@  value_to_double_int (prop_value_t val)
     return double_int_zero;
 }
 
-/* Return the value for the address expression EXPR based on alignment
-   information.  */
+/* Return the value for an expr of type TYPE with alignment ALIGN and offset
+   BITPOS relative to the alignment.  */
 
 static prop_value_t
-get_value_from_alignment (tree expr)
+get_align_value (unsigned int align, tree type, unsigned HOST_WIDE_INT bitpos)
 {
-  tree type = TREE_TYPE (expr);
   prop_value_t val;
-  unsigned HOST_WIDE_INT bitpos;
-  unsigned int align;
-
-  gcc_assert (TREE_CODE (expr) == ADDR_EXPR);
 
-  align = get_object_alignment_1 (TREE_OPERAND (expr, 0), &bitpos);
   val.mask
     = double_int_and_not (POINTER_TYPE_P (type) || TYPE_UNSIGNED (type)
 			  ? double_int_mask (TYPE_PRECISION (type))
@@ -529,6 +523,21 @@  get_value_from_alignment (tree expr)
   return val;
 }
 
+/* Return the value for the address expression EXPR based on alignment
+   information.  */
+
+static prop_value_t
+get_value_from_alignment (tree expr)
+{
+  unsigned int align;
+  unsigned HOST_WIDE_INT bitpos;
+
+  gcc_assert (TREE_CODE (expr) == ADDR_EXPR);
+
+  align = get_object_alignment_1 (TREE_OPERAND (expr, 0), &bitpos);
+  return get_align_value (align, TREE_TYPE (expr), bitpos);
+}
+
 /* Return the value for the tree operand EXPR.  If FOR_BITS_P is true
    return constant bits extracted from alignment information for
    invariant addresses.  */
@@ -540,11 +549,21 @@  get_value_for_expr (tree expr, bool for_
 
   if (TREE_CODE (expr) == SSA_NAME)
     {
+      tree var = SSA_NAME_VAR (expr);
       val = *get_value (expr);
-      if (for_bits_p
-	  && val.lattice_val == CONSTANT
+      if (!for_bits_p)
+	return val;
+
+      if (val.lattice_val == CONSTANT
 	  && TREE_CODE (val.value) == ADDR_EXPR)
 	val = get_value_from_alignment (val.value);
+      else if (val.lattice_val == VARYING
+	       && 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);
     }
   else if (is_gimple_min_invariant (expr)
 	   && (!for_bits_p || TREE_CODE (expr) != ADDR_EXPR))
Index: gcc/testsuite/gcc.dg/pr43814.c
===================================================================
--- /dev/null (new file)
+++ gcc/testsuite/gcc.dg/pr43814.c (revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-ccp1-all" } */
+
+typedef __attribute__((aligned (sizeof (int)))) int aint;
+
+void
+f (aint *a)
+{
+  *(a+1) = 0;
+}
+
+/* { dg-final { scan-tree-dump-times "ALIGN = \[2-9\]\[0-9\]*, MISALIGN = 0" 1 "ccp1"} } */
+/* { dg-final { cleanup-tree-dump "ccp1" } } */
+
Index: gcc/testsuite/gcc.target/arm/pr43814-2.c
===================================================================
--- /dev/null (new file)
+++ gcc/testsuite/gcc.target/arm/pr43814-2.c (revision 0)
@@ -0,0 +1,36 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-expand" } */
+
+typedef unsigned int size_t;
+extern void* memcpy (void *, const void *, size_t);
+
+typedef __attribute__((aligned (sizeof (int))))
+  const unsigned int cst_aligned_uint;
+
+typedef union JValue {
+  void* l;
+} JValue;
+typedef struct Object {
+  int x;
+} Object;
+
+extern __inline__ long long
+dvmGetArgLong (cst_aligned_uint* args, int elem)
+{
+  long long val;
+  memcpy (&val, &args[elem], 8);
+  return val;
+}
+
+void
+Dalvik_sun_misc_Unsafe_getObject (cst_aligned_uint* args, JValue* pResult)
+{
+  Object* obj = (Object*) args[1];
+  long long offset = dvmGetArgLong (args, 2);
+  Object** address = (Object**) (((unsigned char*) obj) + offset);
+  pResult->l = ((void*) *address);
+}
+
+/* { dg-final { scan-rtl-dump-times "memcpy" 0 "expand"} } */
+/* { dg-final { cleanup-tree-dump "expand" } } */
+