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

[Tom de Vries]

On 26/10/11 12:19, Richard Guenther wrote:
 On Tue, Oct 25, 2011 at 2:22 PM, Tom de Vries tom_devr...@mentor.com wrote:
 On 09/24/2011 01:42 PM, Richard Guenther wrote:
 On Sat, Sep 24, 2011 at 11:40 AM, Jakub Jelinek ja...@redhat.com 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.

 Yeah (even if technically invoking undefined behavior in C).  Checking if
 there is a dereference post-dominating function entry with sth like

   FOR_EACH_IMM_USE_STMT (... ptr ...)
  if (stmt_post_dominates_entry  contains derefrence of ptr)
alignment = TYPE_ALIGN (...);

 and otherwise not assuming anything about parameter alignment might work.
 Be careful to check the alignment of the dereference though,

 typedef int int_unaligned __attribute__((aligned(1)));
 int foo (int *p)
 {
   int_unaligned *q = p;
   return *q;
 }

 will be MEM[p] but with (well, hopefully ;)) TYPE_ALIGN of TREE_TYPE 
 (MEM[p])
 being 1.  And yes, you'd have to look into handled-components as well.  I 
 guess
 you'll face similar problems as we do with tree-sra.c
 tree_non_mode_aligned_mem_p
 (you need to assume eventually misaligned accesses the same way expansion
 does for the dereference, otherwise you'll run into issues on
 strict-align targets).

 As that de-refrence thing doesn't really fit the CCP propagation you
 won't be able
 to handle

 int foo (int *p)
 {
   int *q = (char *)p + 3;
   return *q;
 }

 and assume q is aligned (and p is misaligned by 1).

 That is, if the definition of a pointer is post-dominated by a derefrence
 we could assume proper alignment for that pointer (as opposed to just
 special-casing its default definition).  Would be certainly interesting to
 see what kind of fallout we would get from that ;)


 I gave this a try in deduce_alignment_from_dereferences.

 The fall-out I got from this were unaligned dereferenced pointers in
 gcc.c-torture/unsorted/*{cmp,set}.c.

 Bootstrapped and reg-tested on x86_64. Build and reg-tested on MIPS and ARM.

 Ok for trunk?
 
 Can you not do the get_value_from_alignment split (it doesn't look
 necessary to me) and drop the
 
 @@ -541,10 +550,18 @@ get_value_for_expr (tree expr, bool for_
if (TREE_CODE (expr) == SSA_NAME)
  {
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_PTR_INFO (expr) != NULL
 +   SSA_NAME_PTR_INFO (expr)-align  1
 +   SSA_NAME_PTR_INFO (expr)-misalign == 0)
 +   val = get_align_value (SSA_NAME_PTR_INFO (expr)-align * 
 BITS_PER_UNIT,
 +  TREE_TYPE (expr), 0);
  }
 
 hunk?  I'm not sure why it is necessary at all - CCP is the only pass
 computing alignment, so it should simply re-compute the info?
 
 Anyway, it looks unrelated to the purpose of the patch in general.
 

I dropped the code in get_value_for_expr, but kept the factoring of
get_align_value out of get_value_from_alignment . This remains necessary in the
new patch since get_align_value is still called from 2 sites:
get_value_from_alignment and deduce_alignment_from_dereference.

 The error reporting in deduce_alignment_from_dereferences is bogus,
 the programs are undefined only at runtime, so you can at most
 issue a warning.
 

Introduced -Wunaligned-pointer-deref.

 +  /* Needs to be the successor of entry, for CDI_POST_DOMINATORS.  */
 +  entry = single_succ (ENTRY_BLOCK_PTR);
 +
 +  FOR_EACH_BB (bb)
 +{
 +  gimple_stmt_iterator i;
 +
 +  if (!dominated_by_p (CDI_POST_DOMINATORS, entry, bb))
 +   continue;
 
 if you only consider post-dominators of the entry block then just walk
 them directly (first_dom_son / next_dom_son).
 
 + align = TYPE_ALIGN (TREE_TYPE (memref)) / BITS_PER_UNIT;
 + if (align == 1)
 +   continue;
 

I integrated the walking of post-dominators into the bb walk in ccp_initialize.

 I think you want to match what expand thinks of the alignment of this
 memory reference, not just what TYPE_ALIGN says (and yes, that
 needs to be split out somehow, SRA would need this as well).
 

I'm now using get_object_or_type_alignment for MEM_REF.

 + while (TREE_CODE (ptr) == SSA_NAME)
 +   {
 +

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

[Eric Botcazou]

   * expr.c (get_object_or_type_alignment): Remove static.
   * expr.h (get_object_or_type_alignment): Declare.

I did that this morning (and also added an assertion in the function).

-- 
Eric Botcazou

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

[Richard Guenther]

On Tue, Oct 25, 2011 at 2:22 PM, Tom de Vries tom_devr...@mentor.com wrote:
 On 09/24/2011 01:42 PM, Richard Guenther wrote:
 On Sat, Sep 24, 2011 at 11:40 AM, Jakub Jelinek ja...@redhat.com 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.

 Yeah (even if technically invoking undefined behavior in C).  Checking if
 there is a dereference post-dominating function entry with sth like

   FOR_EACH_IMM_USE_STMT (... ptr ...)
      if (stmt_post_dominates_entry  contains derefrence of ptr)
        alignment = TYPE_ALIGN (...);

 and otherwise not assuming anything about parameter alignment might work.
 Be careful to check the alignment of the dereference though,

 typedef int int_unaligned __attribute__((aligned(1)));
 int foo (int *p)
 {
   int_unaligned *q = p;
   return *q;
 }

 will be MEM[p] but with (well, hopefully ;)) TYPE_ALIGN of TREE_TYPE (MEM[p])
 being 1.  And yes, you'd have to look into handled-components as well.  I 
 guess
 you'll face similar problems as we do with tree-sra.c
 tree_non_mode_aligned_mem_p
 (you need to assume eventually misaligned accesses the same way expansion
 does for the dereference, otherwise you'll run into issues on
 strict-align targets).

 As that de-refrence thing doesn't really fit the CCP propagation you
 won't be able
 to handle

 int foo (int *p)
 {
   int *q = (char *)p + 3;
   return *q;
 }

 and assume q is aligned (and p is misaligned by 1).

 That is, if the definition of a pointer is post-dominated by a derefrence
 we could assume proper alignment for that pointer (as opposed to just
 special-casing its default definition).  Would be certainly interesting to
 see what kind of fallout we would get from that ;)


 I gave this a try in deduce_alignment_from_dereferences.

 The fall-out I got from this were unaligned dereferenced pointers in
 gcc.c-torture/unsorted/*{cmp,set}.c.

 Bootstrapped and reg-tested on x86_64. Build and reg-tested on MIPS and ARM.

 Ok for trunk?

Can you not do the get_value_from_alignment split (it doesn't look
necessary to me) and drop the

@@ -541,10 +550,18 @@ get_value_for_expr (tree expr, bool for_
   if (TREE_CODE (expr) == SSA_NAME)
 {
   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_PTR_INFO (expr) != NULL
+   SSA_NAME_PTR_INFO (expr)-align  1
+   SSA_NAME_PTR_INFO (expr)-misalign == 0)
+   val = get_align_value (SSA_NAME_PTR_INFO (expr)-align * BITS_PER_UNIT,
+  TREE_TYPE (expr), 0);
 }

hunk?  I'm not sure why it is necessary at all - CCP is the only pass
computing alignment, so it should simply re-compute the info?

Anyway, it looks unrelated to the purpose of the patch in general.

The error reporting in deduce_alignment_from_dereferences is bogus,
the programs are undefined only at runtime, so you can at most
issue a warning.

+  /* Needs to be the successor of entry, for CDI_POST_DOMINATORS.  */
+  entry = single_succ (ENTRY_BLOCK_PTR);
+
+  FOR_EACH_BB (bb)
+{
+  gimple_stmt_iterator i;
+
+  if (!dominated_by_p (CDI_POST_DOMINATORS, entry, bb))
+   continue;

if you only consider post-dominators of the entry block then just walk
them directly (first_dom_son / next_dom_son).

+ align = TYPE_ALIGN (TREE_TYPE (memref)) / BITS_PER_UNIT;
+ if (align == 1)
+   continue;

I think you want to match what expand thinks of the alignment of this
memory reference, not just what TYPE_ALIGN says (and yes, that
needs to be split out somehow, SRA would need this as well).

+ while (TREE_CODE (ptr) == SSA_NAME)
+   {
+ pi = get_ptr_info (ptr);
+ if (pi-misalign != 0)
+   {
+ error (misaligned pointer dereferenced);
+ break;
+   }

simply looking at pi-misalign is wrong.  pi-align may be bigger
than the align that you computed above, so pi-misalign % align != 0
would be the right check.

+ if (pi-align = align)
+   break;
+ pi-align = align;

and then set pi-misalign to zero here.  But I would initialize the
CCP lattice with this, not set SSA_NAME_PTR_INFO, 

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

[Tom de Vries]

On 09/24/2011 01:42 PM, Richard Guenther wrote:
 On Sat, Sep 24, 2011 at 11:40 AM, Jakub Jelinek ja...@redhat.com 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.
 
 Yeah (even if technically invoking undefined behavior in C).  Checking if
 there is a dereference post-dominating function entry with sth like
 
   FOR_EACH_IMM_USE_STMT (... ptr ...)
  if (stmt_post_dominates_entry  contains derefrence of ptr)
alignment = TYPE_ALIGN (...);
 
 and otherwise not assuming anything about parameter alignment might work.
 Be careful to check the alignment of the dereference though,
 
 typedef int int_unaligned __attribute__((aligned(1)));
 int foo (int *p)
 {
   int_unaligned *q = p;
   return *q;
 }
 
 will be MEM[p] but with (well, hopefully ;)) TYPE_ALIGN of TREE_TYPE (MEM[p])
 being 1.  And yes, you'd have to look into handled-components as well.  I 
 guess
 you'll face similar problems as we do with tree-sra.c
 tree_non_mode_aligned_mem_p
 (you need to assume eventually misaligned accesses the same way expansion
 does for the dereference, otherwise you'll run into issues on
 strict-align targets).
 
 As that de-refrence thing doesn't really fit the CCP propagation you
 won't be able
 to handle
 
 int foo (int *p)
 {
   int *q = (char *)p + 3;
   return *q;
 }
 
 and assume q is aligned (and p is misaligned by 1).
 
 That is, if the definition of a pointer is post-dominated by a derefrence
 we could assume proper alignment for that pointer (as opposed to just
 special-casing its default definition).  Would be certainly interesting to
 see what kind of fallout we would get from that ;)
 

I gave this a try in deduce_alignment_from_dereferences.

The fall-out I got from this were unaligned dereferenced pointers in
gcc.c-torture/unsorted/*{cmp,set}.c.

Bootstrapped and reg-tested on x86_64. Build and reg-tested on MIPS and ARM.

Ok for trunk?

Thanks,
- Tom

 Richard.
 
Jakub


2011-10-25  Tom de Vries t...@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
pointers with ptr_info-align set.
(deduce_alignment_from_dereferences): New function.
(do_ssa_ccp): Calculate post-dominance info and call
deduce_alignment_from_dereferences.

* gcc/testsuite/gcc.target/arm/pr43814-2.c: New test.
* gcc.c-torture/unsorted/HIcmp.c: Fix unaligned pointer.
* gcc.c-torture/unsorted/HIset.c: Same.
* gcc.c-torture/unsorted/SIcmp.c: Same.
* gcc.c-torture/unsorted/SIset.c: Same.
* gcc.c-torture/unsorted/SFset.c: Same.
* gcc.c-torture/unsorted/UHIcmp.c: Same.
* gcc.c-torture/unsorted/USIcmp.c: Same.
* gcc.c-torture/unsorted/DFcmp.c: Same.
Index: gcc/tree-ssa-ccp.c
===
--- gcc/tree-ssa-ccp.c (revision 180237)
+++ 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
  

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

[Richard Guenther]

On Wed, Oct 19, 2011 at 1:58 AM, Maxim Kuvyrkov ma...@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_devr...@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.

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

[Maxim Kuvyrkov]

On 29/09/2011, at 10:23 PM, Richard Guenther wrote:

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

Re: [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 vr...@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 t...@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.

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 

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

[Richard Guenther]

On Wed, Sep 28, 2011 at 10:08 PM, Jakub Jelinek ja...@redhat.com wrote:
 On Wed, Sep 28, 2011 at 09:56:27PM +0200, Richard Guenther wrote:
 There is nothing like very likely aligned ;)  Note that what is new is

 On non-strict aligned targets there is no reason not to have something like
 very likely aligned.  You would expand stores/loads as if it was aligned
 in that case, and if it isn't, all you'd need to ensure from that is that
 you don't derive properties about the pointer value from the likely
 aligned info, only from alignment.

As if aligned, say, using movaps? ;)  Then we can as well derive
properties about the pointer value.

Note that I don't see a real difference between expanding a load/store
assuming the pointer is aligned from deriving properties about the pointer
value.  To support existing questionable code I'd do both only if
there is a dereference post-dominating the pointer assignment of course.

 Very likely aligned is interesting to the vectorizer too, if it is very
 likely something is sufficiently aligned, the vectorizer could decide to
 assume the alignment in the vectorized loop and add the check for the
 alignment to the loop guards.  In the likely case the vectorized loop would
 be used (if other guards were true too), in the unlikely case it is
 unaligned it would just use a slower loop.

 that we now no longer assume alignment by default (we did in the past)
 and that we derive properties about the pointer _value_ from alignment.

 I think we can derive pointer values when we see dereferences, the
 code wouldn't be portable to strict-alignment targets otherwise.  We

 But any references?  If you have
 int foo (int *p)
 {
  memcpy (p, a, 1);
  return ((uintptr_t) p  3) == 0;
 }
 then even if p isn't aligned, this could work even on strict aligned
 targets.

Well, the above isn't a dereference.  Or rather, if it is, it's a dereference
through a type with assumed alignment of 1 byte.

 Anyway, the arbitrary value in a pointer thing is much more important then
 the rest, so having the dominating dereference test is very important.

Yes.

Richard.

        Jakub

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

[Jakub Jelinek]

On Thu, Sep 29, 2011 at 11:11:12AM +0200, Richard Guenther wrote:
 On Wed, Sep 28, 2011 at 10:08 PM, Jakub Jelinek ja...@redhat.com wrote:
  On Wed, Sep 28, 2011 at 09:56:27PM +0200, Richard Guenther wrote:
  There is nothing like very likely aligned ;)  Note that what is new is
 
  On non-strict aligned targets there is no reason not to have something like
  very likely aligned.  You would expand stores/loads as if it was aligned
  in that case, and if it isn't, all you'd need to ensure from that is that
  you don't derive properties about the pointer value from the likely
  aligned info, only from alignment.
 
 As if aligned, say, using movaps? ;)  Then we can as well derive
 properties about the pointer value.
 
 Note that I don't see a real difference between expanding a load/store
 assuming the pointer is aligned from deriving properties about the pointer
 value.  To support existing questionable code I'd do both only if
 there is a dereference post-dominating the pointer assignment of course.

I was talking about e.g. PR49442, where using the unaligned stores has
severe performance hit.  If compiler was hinted that the store pointers are
likely aligned, it could version on the alignment.

  But any references?  If you have
  int foo (int *p)
  {
   memcpy (p, a, 1);
   return ((uintptr_t) p  3) == 0;
  }
  then even if p isn't aligned, this could work even on strict aligned
  targets.
 
 Well, the above isn't a dereference.  Or rather, if it is, it's a 
 dereference
 through a type with assumed alignment of 1 byte.

If the dereference is only through the declared type of the parameter, I
feel much safer about it.  But I thought this thread was exactly about
memcpy/memset with int * pointer not dereferenced in any other way.

Jakub

Re: [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_devr...@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 vr...@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 t...@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.

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

[Richard Guenther]

On Thu, Sep 29, 2011 at 11:20 AM, Jakub Jelinek ja...@redhat.com wrote:
 On Thu, Sep 29, 2011 at 11:11:12AM +0200, Richard Guenther wrote:
 On Wed, Sep 28, 2011 at 10:08 PM, Jakub Jelinek ja...@redhat.com wrote:
  On Wed, Sep 28, 2011 at 09:56:27PM +0200, Richard Guenther wrote:
  There is nothing like very likely aligned ;)  Note that what is new is
 
  On non-strict aligned targets there is no reason not to have something like
  very likely aligned.  You would expand stores/loads as if it was aligned
  in that case, and if it isn't, all you'd need to ensure from that is that
  you don't derive properties about the pointer value from the likely
  aligned info, only from alignment.

 As if aligned, say, using movaps? ;)  Then we can as well derive
 properties about the pointer value.

 Note that I don't see a real difference between expanding a load/store
 assuming the pointer is aligned from deriving properties about the pointer
 value.  To support existing questionable code I'd do both only if
 there is a dereference post-dominating the pointer assignment of course.

 I was talking about e.g. PR49442, where using the unaligned stores has
 severe performance hit.  If compiler was hinted that the store pointers are
 likely aligned, it could version on the alignment.

Well, it could do so anyway?  It just doesn't as the vectorizer cost model
and hw description says using misaligned moves is just fine.

  But any references?  If you have
  int foo (int *p)
  {
   memcpy (p, a, 1);
   return ((uintptr_t) p  3) == 0;
  }
  then even if p isn't aligned, this could work even on strict aligned
  targets.

 Well, the above isn't a dereference.  Or rather, if it is, it's a 
 dereference
 through a type with assumed alignment of 1 byte.

 If the dereference is only through the declared type of the parameter, I
 feel much safer about it.  But I thought this thread was exactly about
 memcpy/memset with int * pointer not dereferenced in any other way.

Maybe that was the PR the patch is about, but surely we can't treat
a memcpy (p, ..) like *p.  Which means we'd use the declared type
of p only.  And we can do so only for parameters (intermediate conversions
are dropped), which would make the case apply to artificial testcases only
(and thus not worth).

Richard.

        Jakub

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

[Richard Guenther]

On Thu, Sep 29, 2011 at 12:51 PM, Bernd Schmidt ber...@codesourcery.com wrote:
 On 09/29/11 11:26, Richard Guenther wrote:
 Maybe that was the PR the patch is about, but surely we can't treat
 a memcpy (p, ..) like *p.  Which means we'd use the declared type
 of p only.  And we can do so only for parameters (intermediate conversions
 are dropped), which would make the case apply to artificial testcases only
 (and thus not worth).

 Didn't we used to do exactly that, though?

Yes we did.  The point is this is all language semantics and we are not
good at expressing it given the various GCC extensions we have.

struct { int i; } __attribute__((aligned(1))) x;

and x-i will have type int *, not int __attribute__((aligned(1))) *.
Basically because we share FIELD_DECLs for type variants and
alignment consitutes a variant, the FIELD_DECL has natural
alignment and whenever the context (that we access a component of x)
vanishes the alignment information vanishes.

Thus, the middle-end cannot reasonably derive anything from type
alignment.  Because the frontends get it wrong.  Which means we
used to miscompile valid code (using GCC extensions, of course).

Richard.

 Googling shows

 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=325670

 but I seem to remember PRs in gcc bugzilla marked not a bug. Or maybe my
 memory is playing tricks on me.


 Bernd

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

[Carrot Wei]

Hi Tom

What's the behavior of your patch to the following case

typedef int int_unaligned __attribute__((aligned(1)));
int foo (int_unaligned *p)
{
  return *p;
}

thanks
Carrot

On Tue, Sep 20, 2011 at 7:13 PM, Tom de Vries vr...@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?

 Thanks,
 - Tom

 2011-09-20  Tom de Vries t...@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.
        * common.opt (faligned-pointer-argument): New option.
        * doc/invoke.texi (Optimization Options): Add
        -faligned-pointer-argument.
        (-faligned-pointer-argument): New item.

        * gcc/testsuite/gcc.dg/pr43814.c: New test.
        * gcc/testsuite/gcc.target/arm/pr43814-2.c: New test.

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

[Tom de Vries]

On 09/28/2011 03:57 PM, Carrot Wei wrote:
 Hi Tom
 
 What's the behavior of your patch to the following case
 
 typedef int int_unaligned __attribute__((aligned(1)));
 int foo (int_unaligned *p)
 {
   return *p;
 }
 

I modified the example slightly:

test.c:
...
typedef int __attribute__((aligned(2))) int_unaligned;
int foo (int_unaligned *p)
{
  return *(p+1);
}
...

test.c.023t.ccp1:
...
  # PT = anything
  # ALIGN = 2, MISALIGN = 0
  D.2723_2 = pD.1604_1(D) + 4;
...

Thanks,
- Tom

 thanks
 Carrot
 
 On Tue, Sep 20, 2011 at 7:13 PM, Tom de Vries vr...@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?

 Thanks,
 - Tom

 2011-09-20  Tom de Vries t...@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.
* common.opt (faligned-pointer-argument): New option.
* doc/invoke.texi (Optimization Options): Add
-faligned-pointer-argument.
(-faligned-pointer-argument): New item.

* gcc/testsuite/gcc.dg/pr43814.c: New test.
* gcc/testsuite/gcc.target/arm/pr43814-2.c: New test.

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

[Maxim Kuvyrkov]

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

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

[Maxim Kuvyrkov]

On 29/09/2011, at 7:35 AM, David Miller wrote:

 From: Maxim Kuvyrkov ma...@codesourcery.com
 Date: Thu, 29 Sep 2011 07:29:12 +1300
 
 GLIBC patch to fix locale_t definition is attached.
 
 Isn't this going to result in byte loads being used to dereference
 all locale_t pointers on targets like sparc and mips?

Yes, that's the price for binary compatibility for comparing a pointer to -1.  
I just hope that no common program has locale functions on its critical path.

Thank you,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics

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

[David Miller]

From: Maxim Kuvyrkov ma...@codesourcery.com
Date: Thu, 29 Sep 2011 07:40:55 +1300

 On 29/09/2011, at 7:35 AM, David Miller wrote:
 
 From: Maxim Kuvyrkov ma...@codesourcery.com
 Date: Thu, 29 Sep 2011 07:29:12 +1300
 
 GLIBC patch to fix locale_t definition is attached.
 
 Isn't this going to result in byte loads being used to dereference
 all locale_t pointers on targets like sparc and mips?
 
 Yes, that's the price for binary compatibility for comparing a pointer to -1. 
  I just hope that no common program has locale functions on its critical path.

I personally don't think this is acceptable, critical path or not.

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

[Maxim Kuvyrkov]

On 29/09/2011, at 7:41 AM, David Miller wrote:

 From: Maxim Kuvyrkov ma...@codesourcery.com
 Date: Thu, 29 Sep 2011 07:40:55 +1300
 
 On 29/09/2011, at 7:35 AM, David Miller wrote:
 
 From: Maxim Kuvyrkov ma...@codesourcery.com
 Date: Thu, 29 Sep 2011 07:29:12 +1300
 
 GLIBC patch to fix locale_t definition is attached.
 
 Isn't this going to result in byte loads being used to dereference
 all locale_t pointers on targets like sparc and mips?
 
 Yes, that's the price for binary compatibility for comparing a pointer to 
 -1.  I just hope that no common program has locale functions on its critical 
 path.
 
 I personally don't think this is acceptable, critical path or not.

OK.  Do you have an alternative suggestion that would fix non-portable use of 
locale_t?

Thank you,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics

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

[David Miller]

From: Maxim Kuvyrkov ma...@codesourcery.com
Date: Thu, 29 Sep 2011 07:45:17 +1300

 OK.  Do you have an alternative suggestion that would fix non-portable use of 
 locale_t?

Don't optimize something that is invalidated by a quite common practice?

What about people who encode invalid pointers with 0xdeadbeef, do we need
to audit every source tree that does this too?  This invalidates the
optimization's preconditions as well.

You're not going to eradicate all the code in the world which uses unaligned
constants to encode pointers to make them have special meanings in certain
situations.

We use the -1 thing in the Linux kernel too I believe.  I'd go so far as
to say this kind of thing is pervasive.

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

[David Miller]

From: Maxim Kuvyrkov ma...@codesourcery.com
Date: Thu, 29 Sep 2011 07:58:01 +1300

 To summarize, your opinion seems to be to not enable the
 optimization by default, correct?

Yes, and I don't think we ever could do so.

BTW, another common paradigm is using the always clear bits of a
pointer to encode state bits.

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

[Jakub Jelinek]

On Wed, Sep 28, 2011 at 03:00:09PM -0400, David Miller wrote:
 From: Maxim Kuvyrkov ma...@codesourcery.com
 Date: Thu, 29 Sep 2011 07:58:01 +1300
 
  To summarize, your opinion seems to be to not enable the
  optimization by default, correct?
 
 Yes, and I don't think we ever could do so.
 
 BTW, another common paradigm is using the always clear bits of a
 pointer to encode state bits.

Yeah, I think it is a bad optimization that is going to break lots of code
and the glibc patch is definitely unacceptable (and not doing what you think
it is doing, your change didn't do anything at all, as the aligned attribute
applies to the pointer type and not to the pointed type).

The alignment of the argument pointer can be IMHO only hard assumed
if there is a load or store using that type dominating the spot where you
are checking it, and the target is strict alignment target.
Then we can both assume that alignment, optimize away tests on the low bits
etc.
Otherwise, what you could do is just use the pointed type's alignment as
a hint, likely alignment, e.g. on non-strict alignment target you could
expand code optimistically assuming that it is very likely aligned, but
still shouldn't optimize away low bits tests.

Jakub

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

[Richard Guenther]

On Wed, Sep 28, 2011 at 9:13 PM, Jakub Jelinek ja...@redhat.com wrote:
 On Wed, Sep 28, 2011 at 03:00:09PM -0400, David Miller wrote:
 From: Maxim Kuvyrkov ma...@codesourcery.com
 Date: Thu, 29 Sep 2011 07:58:01 +1300

  To summarize, your opinion seems to be to not enable the
  optimization by default, correct?

 Yes, and I don't think we ever could do so.

 BTW, another common paradigm is using the always clear bits of a
 pointer to encode state bits.

 Yeah, I think it is a bad optimization that is going to break lots of code
 and the glibc patch is definitely unacceptable (and not doing what you think
 it is doing, your change didn't do anything at all, as the aligned attribute
 applies to the pointer type and not to the pointed type).

 The alignment of the argument pointer can be IMHO only hard assumed
 if there is a load or store using that type dominating the spot where you
 are checking it, and the target is strict alignment target.
 Then we can both assume that alignment, optimize away tests on the low bits
 etc.
 Otherwise, what you could do is just use the pointed type's alignment as
 a hint, likely alignment, e.g. on non-strict alignment target you could
 expand code optimistically assuming that it is very likely aligned, but
 still shouldn't optimize away low bits tests.

There is nothing like very likely aligned ;)  Note that what is new is
that we now no longer assume alignment by default (we did in the past)
and that we derive properties about the pointer _value_ from alignment.

I think we can derive pointer values when we see dereferences, the
code wouldn't be portable to strict-alignment targets otherwise.  We
can offer a -fno-strict-alignment option to disable deriving alignment
from types.

Richard.

        Jakub

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

[Jakub Jelinek]

On Wed, Sep 28, 2011 at 09:56:27PM +0200, Richard Guenther wrote:
 There is nothing like very likely aligned ;)  Note that what is new is

On non-strict aligned targets there is no reason not to have something like
very likely aligned.  You would expand stores/loads as if it was aligned
in that case, and if it isn't, all you'd need to ensure from that is that
you don't derive properties about the pointer value from the likely
aligned info, only from alignment.

Very likely aligned is interesting to the vectorizer too, if it is very
likely something is sufficiently aligned, the vectorizer could decide to
assume the alignment in the vectorized loop and add the check for the
alignment to the loop guards.  In the likely case the vectorized loop would
be used (if other guards were true too), in the unlikely case it is
unaligned it would just use a slower loop.

 that we now no longer assume alignment by default (we did in the past)
 and that we derive properties about the pointer _value_ from alignment.
 
 I think we can derive pointer values when we see dereferences, the
 code wouldn't be portable to strict-alignment targets otherwise.  We

But any references?  If you have
int foo (int *p)
{
  memcpy (p, a, 1);
  return ((uintptr_t) p  3) == 0;
}
then even if p isn't aligned, this could work even on strict aligned
targets.

Anyway, the arbitrary value in a pointer thing is much more important then
the rest, so having the dominating dereference test is very important.

Jakub

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

[Mike Stump]

On Sep 20, 2011, at 4:13 AM, Tom de Vries wrote:
 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'm not a huge fan of an option that is very hard to use.  I think this option 
would be hard to use.  I'd rather have a port just assert that no code will be 
compiled that is weird in this way, then the front end can check for weird 
values on int to pointer conversions with constants and complain about the 
code, if they tried it.  If Android is safe in this respect, then, they can 
just turn it on, and then force anyone porting software to their platform to 
`fix' their code.  For systems that are clean, and new systems, we can 
recommend they set the option on.  For legacy systems that don't want to change 
or just want to compile legacy software, well, they can opt out.

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

[Tom de Vries]

On 09/24/2011 11:31 AM, Richard Guenther wrote:
 On Tue, Sep 20, 2011 at 1:13 PM, Tom de Vries vr...@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.

OK for trunk?

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 t...@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.
Index: gcc/tree-cfgcleanup.c
===
--- gcc/tree-cfgcleanup.c	(revision 173703)
+++ gcc/tree-cfgcleanup.c	(working copy)
@@ -641,6 +641,552 @@ cleanup_omp_return (basic_block bb)
   return true;
 }
 
+/* Returns true if S contains (I1, I2).  */
+
+static bool
+int_int_splay_lookup (splay_tree s, unsigned int i1, unsigned int i2)
+{
+  splay_tree_node node;
+
+  if (s == NULL)
+return false;
+
+  node = splay_tree_lookup (s, i1);
+  return node  node-value == i2;
+}
+
+/* Attempts to insert (I1, I2) into *S.  Returns true if successful.
+   Allocates *S if necessary.  */
+
+static bool
+int_int_splay_insert (splay_tree *s, unsigned int i1 , unsigned int i2)
+{
+  if (*s != NULL)
+{
+  /* Check for existing element, which would otherwise be silently
+	 overwritten by splay_tree_insert.  */
+  if (splay_tree_lookup (*s, i1))
+	return false;
+}
+  else
+*s = splay_tree_new (splay_tree_compare_ints, 0, 0);
+
+  splay_tree_insert (*s, i1, i2);
+  return true;
+}
+
+/* Returns 0 if (NODE-value, NODE-key) is an element of S.  Otherwise,
+   returns 1.  */
+
+static int
+int_int_splay_node_contained_in (splay_tree_node node, void *s)
+{
+  splay_tree_node snode = splay_tree_lookup ((splay_tree)s, node-key);
+  return (!snode || node-value != snode-value) ? 1 : 0;
+}
+
+/* Returns true if all elements of S1 are also in S2.  */
+
+static bool
+int_int_splay_contained_in (splay_tree s1, splay_tree s2)
+{
+  

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

[David Miller]

From: Mike Stump mikest...@comcast.net
Date: Wed, 28 Sep 2011 15:19:10 -0700

 If Android is safe in this respect, then, they can just turn it on,
 and then force anyone porting software to their platform to `fix'
 their code.

They'd have to then know to turn this option off when building the kernel,
which does use such constructs extensively.

I think this whole idea has too many downsides to be considered seriously.

People write problems like this, lots of people.  It's a pervasive
technique to encode boolean state into the low bits of a pointer or to
represent special token pointers using integers such as -1.

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

[Jakub Jelinek]

On Wed, Sep 28, 2011 at 06:43:04PM -0400, David Miller wrote:
 From: Mike Stump mikest...@comcast.net
 Date: Wed, 28 Sep 2011 15:19:10 -0700
 
  If Android is safe in this respect, then, they can just turn it on,
  and then force anyone porting software to their platform to `fix'
  their code.
 
 They'd have to then know to turn this option off when building the kernel,
 which does use such constructs extensively.
 
 I think this whole idea has too many downsides to be considered seriously.
 
 People write problems like this, lots of people.  It's a pervasive
 technique to encode boolean state into the low bits of a pointer or to
 represent special token pointers using integers such as -1.

Or 1.  Just do
grep '\*)[[:blank:]]*1' *.[chS]
to see how often an integer value is stored into a pointer.
And it is not just void * pointers, it is struct cgraph_node *
or struct varpool_node * too and the pointed types there certainly
have higher alignment than 1.
Now repeat the same with google codesearch.

Jakub

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

[Richard Guenther]

On Tue, Sep 20, 2011 at 1:13 PM, Tom de Vries vr...@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.

Richard.

 Thanks,
 - Tom

 2011-09-20  Tom de Vries t...@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.
        * common.opt (faligned-pointer-argument): New option.
        * doc/invoke.texi (Optimization Options): Add
        -faligned-pointer-argument.
        (-faligned-pointer-argument): New item.

        * gcc/testsuite/gcc.dg/pr43814.c: New test.
        * gcc/testsuite/gcc.target/arm/pr43814-2.c: New test.

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

[Jakub Jelinek]

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.

Jakub

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

[Richard Guenther]

On Sat, Sep 24, 2011 at 11:40 AM, Jakub Jelinek ja...@redhat.com 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.

Yeah (even if technically invoking undefined behavior in C).  Checking if
there is a dereference post-dominating function entry with sth like

  FOR_EACH_IMM_USE_STMT (... ptr ...)
 if (stmt_post_dominates_entry  contains derefrence of ptr)
   alignment = TYPE_ALIGN (...);

and otherwise not assuming anything about parameter alignment might work.
Be careful to check the alignment of the dereference though,

typedef int int_unaligned __attribute__((aligned(1)));
int foo (int *p)
{
  int_unaligned *q = p;
  return *q;
}

will be MEM[p] but with (well, hopefully ;)) TYPE_ALIGN of TREE_TYPE (MEM[p])
being 1.  And yes, you'd have to look into handled-components as well.  I guess
you'll face similar problems as we do with tree-sra.c
tree_non_mode_aligned_mem_p
(you need to assume eventually misaligned accesses the same way expansion
does for the dereference, otherwise you'll run into issues on
strict-align targets).

As that de-refrence thing doesn't really fit the CCP propagation you
won't be able
to handle

int foo (int *p)
{
  int *q = (char *)p + 3;
  return *q;
}

and assume q is aligned (and p is misaligned by 1).

That is, if the definition of a pointer is post-dominated by a derefrence
we could assume proper alignment for that pointer (as opposed to just
special-casing its default definition).  Would be certainly interesting to
see what kind of fallout we would get from that ;)

Richard.

        Jakub