[PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

under some circumstances involving user specified alignment and/or
packed attributes, SRA can create a misaligned MEM_REF.  As the
testcase demonstrates, it is not enough to not consider variables with
these type attributes, mainly because we might attempt to load/store
the scalar replacements from/to right/left sides of original aggregate
assignments which might be misaligned.

I'm wondering whether this approach isn't too heavy-handed but I have
not been able to convince myself that anything short of this is
sufficient, esp. in presence of the all-time-SRA-favorite type-casts,
one-field-structures and unions.  At the very least I therefore do
this only for strict-alignment architectures, where this is actually
an issue.

I have verified the testcase fails with a "bus error" on sparc64 and
passes when the patch is applied.  I have run make -k test for c and
c++ on sparc64-linux without any issues as well as traditional
bootstrap and full testsuite run on x86_64-linux.  OK for trunk and
for 4.6 when unfrozen?

Thanks,

Martin


2011-06-24  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/49094
	* tree-sra.c (potential_alignment_issues): New function.
	(build_accesses_from_assign): Use it.

	* testsuite/gcc.dg/tree-ssa/pr49094.c: New test.


Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -1023,6 +1023,33 @@ disqualify_ops_if_throwing_stmt (gimple
   return false;
 }
 
+/* Return true iff type of EXP or any of the types it is based on are
+   user-aligned and packed.  */
+
+static bool
+potential_alignment_issues (tree exp)
+{
+  if (!STRICT_ALIGNMENT)
+    return false;
+
+  while (true)
+    {
+      tree type = TREE_TYPE (exp);
+
+      if (TYPE_USER_ALIGN (type)
+	  || TYPE_PACKED (type))
+	return true;
+
+      if (handled_component_p (exp))
+	exp = TREE_OPERAND (exp, 0);
+      else
+	break;
+    }
+
+  return false;
+}
+
+
 /* Scan expressions occuring in STMT, create access structures for all accesses
    to candidates for scalarization and remove those candidates which occur in
    statements or expressions that prevent them from being split apart.  Return
@@ -1047,7 +1074,10 @@ build_accesses_from_assign (gimple stmt)
   lacc = build_access_from_expr_1 (lhs, stmt, true);
 
   if (lacc)
-    lacc->grp_assignment_write = 1;
+    {
+      lacc->grp_assignment_write = 1;
+      lacc->grp_unscalarizable_region |= potential_alignment_issues (rhs);
+    }
 
   if (racc)
     {
@@ -1055,6 +1085,7 @@ build_accesses_from_assign (gimple stmt)
       if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt)
 	  && !is_gimple_reg_type (racc->type))
 	bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
+      racc->grp_unscalarizable_region |= potential_alignment_issues (lhs);
     }
 
   if (lacc && racc
Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options "-O" } */
+
+struct in_addr {
+	unsigned int s_addr;
+};
+
+struct ip {
+	unsigned char ip_p;
+	unsigned short ip_sum;
+	struct	in_addr ip_src,ip_dst;
+} __attribute__ ((aligned(1), packed));
+
+struct ip ip_fw_fwd_addr;
+
+int test_alignment( char *m )
+{
+  struct ip *ip = (struct ip *) m;
+  struct in_addr pkt_dst;
+  pkt_dst = ip->ip_dst ;
+  if( pkt_dst.s_addr == 0 )
+    return 1;
+  else
+    return 0;
+}
+
+int __attribute__ ((noinline, noclone))
+intermediary (char *p)
+{
+  return test_alignment (p);
+}
+
+int
+main (int argc, char *argv[])
+{
+  ip_fw_fwd_addr.ip_dst.s_addr = 1;
+  return intermediary ((void *) &ip_fw_fwd_addr);
+}

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4761 bytes --]

On Sun, 26 Jun 2011, Martin Jambor wrote:

> Hi,
> 
> under some circumstances involving user specified alignment and/or
> packed attributes, SRA can create a misaligned MEM_REF.  As the
> testcase demonstrates, it is not enough to not consider variables with
> these type attributes, mainly because we might attempt to load/store
> the scalar replacements from/to right/left sides of original aggregate
> assignments which might be misaligned.
> 
> I'm wondering whether this approach isn't too heavy-handed but I have
> not been able to convince myself that anything short of this is
> sufficient, esp. in presence of the all-time-SRA-favorite type-casts,
> one-field-structures and unions.  At the very least I therefore do
> this only for strict-alignment architectures, where this is actually
> an issue.
> 
> I have verified the testcase fails with a "bus error" on sparc64 and
> passes when the patch is applied.  I have run make -k test for c and
> c++ on sparc64-linux without any issues as well as traditional
> bootstrap and full testsuite run on x86_64-linux.  OK for trunk and
> for 4.6 when unfrozen?
> 
> Thanks,
> 
> Martin
> 
> 
> 2011-06-24  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/49094
> 	* tree-sra.c (potential_alignment_issues): New function.
> 	(build_accesses_from_assign): Use it.
> 
> 	* testsuite/gcc.dg/tree-ssa/pr49094.c: New test.
> 
> 
> Index: src/gcc/tree-sra.c
> ===================================================================
> --- src.orig/gcc/tree-sra.c
> +++ src/gcc/tree-sra.c
> @@ -1023,6 +1023,33 @@ disqualify_ops_if_throwing_stmt (gimple
>    return false;
>  }
>  
> +/* Return true iff type of EXP or any of the types it is based on are
> +   user-aligned and packed.  */
> +
> +static bool
> +potential_alignment_issues (tree exp)
> +{
> +  if (!STRICT_ALIGNMENT)
> +    return false;
> +
> +  while (true)
> +    {
> +      tree type = TREE_TYPE (exp);
> +
> +      if (TYPE_USER_ALIGN (type)
> +	  || TYPE_PACKED (type))
> +	return true;
> +
> +      if (handled_component_p (exp))
> +	exp = TREE_OPERAND (exp, 0);
> +      else
> +	break;
> +    }

I think you want something like

static bool
tree_non_mode_aligned_mem_p (tree exp)
{
  enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
  unsigned int align;

  if (mode == BLKmode
      || !STRICT_ALIGNMENT)
    return false;

  align = get_object_alignment (exp, BIGGEST_ALIGNMENT);
  if (GET_MODE_ALIGNMENT (mode) > align)
    return true;

  return false;
}

as for STRICT_ALIGNMENT targets we assume that the loads/stores SRA
inserts have the alignment of the mode.

Richard.

> +  return false;
> +}
> +
> +
>  /* Scan expressions occuring in STMT, create access structures for all accesses
>     to candidates for scalarization and remove those candidates which occur in
>     statements or expressions that prevent them from being split apart.  Return
> @@ -1047,7 +1074,10 @@ build_accesses_from_assign (gimple stmt)
>    lacc = build_access_from_expr_1 (lhs, stmt, true);
>  
>    if (lacc)
> -    lacc->grp_assignment_write = 1;
> +    {
> +      lacc->grp_assignment_write = 1;
> +      lacc->grp_unscalarizable_region |= potential_alignment_issues (rhs);
> +    }
>  
>    if (racc)
>      {
> @@ -1055,6 +1085,7 @@ build_accesses_from_assign (gimple stmt)
>        if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt)
>  	  && !is_gimple_reg_type (racc->type))
>  	bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
> +      racc->grp_unscalarizable_region |= potential_alignment_issues (lhs);
>      }
>  
>    if (lacc && racc
> Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
> @@ -0,0 +1,38 @@
> +/* { dg-do run } */
> +/* { dg-options "-O" } */
> +
> +struct in_addr {
> +	unsigned int s_addr;
> +};
> +
> +struct ip {
> +	unsigned char ip_p;
> +	unsigned short ip_sum;
> +	struct	in_addr ip_src,ip_dst;
> +} __attribute__ ((aligned(1), packed));
> +
> +struct ip ip_fw_fwd_addr;
> +
> +int test_alignment( char *m )
> +{
> +  struct ip *ip = (struct ip *) m;
> +  struct in_addr pkt_dst;
> +  pkt_dst = ip->ip_dst ;
> +  if( pkt_dst.s_addr == 0 )
> +    return 1;
> +  else
> +    return 0;
> +}
> +
> +int __attribute__ ((noinline, noclone))
> +intermediary (char *p)
> +{
> +  return test_alignment (p);
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  ip_fw_fwd_addr.ip_dst.s_addr = 1;
> +  return intermediary ((void *) &ip_fw_fwd_addr);
> +}
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

On Mon, Jun 27, 2011 at 03:18:01PM +0200, Richard Guenther wrote:
> On Sun, 26 Jun 2011, Martin Jambor wrote:
> 
> > Hi,
> > 
> > under some circumstances involving user specified alignment and/or
> > packed attributes, SRA can create a misaligned MEM_REF.  As the
> > testcase demonstrates, it is not enough to not consider variables with
> > these type attributes, mainly because we might attempt to load/store
> > the scalar replacements from/to right/left sides of original aggregate
> > assignments which might be misaligned.
> > 

...

> 
> I think you want something like
> 
> static bool
> tree_non_mode_aligned_mem_p (tree exp)
> {
>   enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
>   unsigned int align;
> 
>   if (mode == BLKmode
>       || !STRICT_ALIGNMENT)
>     return false;
> 
>   align = get_object_alignment (exp, BIGGEST_ALIGNMENT);
>   if (GET_MODE_ALIGNMENT (mode) > align)
>     return true;
> 
>   return false;
> }
> 
> as for STRICT_ALIGNMENT targets we assume that the loads/stores SRA
> inserts have the alignment of the mode.
> 

I admit to be surprised this works, I did not know aggregates could
have non-BLK modes.  Anyway, it does, and so I intend to commit the
following  this evening, after a testsuite run on sparc64.  Please
stop me if the previous message was not a pre-approval of sorts.

Thanks a lot,

Martin


2011-06-28  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/49094
	* tree-sra.c (tree_non_mode_aligned_mem_p): New function.
	(build_accesses_from_assign): Use it.

	* testsuite/gcc.dg/tree-ssa/pr49094.c: New test.


Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -1050,6 +1050,25 @@ disqualify_ops_if_throwing_stmt (gimple
   return false;
 }
 
+/* Return true iff type of EXP is not sufficiently aligned.  */
+
+static bool
+tree_non_mode_aligned_mem_p (tree exp)
+{
+  enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
+  unsigned int align;
+
+  if (mode == BLKmode
+      || !STRICT_ALIGNMENT)
+    return false;
+
+  align = get_object_alignment (exp, BIGGEST_ALIGNMENT);
+  if (GET_MODE_ALIGNMENT (mode) > align)
+    return true;
+
+  return false;
+}
+
 /* Scan expressions occuring in STMT, create access structures for all accesses
    to candidates for scalarization and remove those candidates which occur in
    statements or expressions that prevent them from being split apart.  Return
@@ -1074,7 +1093,10 @@ build_accesses_from_assign (gimple stmt)
   lacc = build_access_from_expr_1 (lhs, stmt, true);
 
   if (lacc)
-    lacc->grp_assignment_write = 1;
+    {
+      lacc->grp_assignment_write = 1;
+      lacc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (rhs);
+    }
 
   if (racc)
     {
@@ -1082,6 +1104,7 @@ build_accesses_from_assign (gimple stmt)
       if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt)
 	  && !is_gimple_reg_type (racc->type))
 	bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
+      racc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (lhs);
     }
 
   if (lacc && racc
Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options "-O" } */
+
+struct in_addr {
+	unsigned int s_addr;
+};
+
+struct ip {
+	unsigned char ip_p;
+	unsigned short ip_sum;
+	struct	in_addr ip_src,ip_dst;
+} __attribute__ ((aligned(1), packed));
+
+struct ip ip_fw_fwd_addr;
+
+int test_alignment( char *m )
+{
+  struct ip *ip = (struct ip *) m;
+  struct in_addr pkt_dst;
+  pkt_dst = ip->ip_dst ;
+  if( pkt_dst.s_addr == 0 )
+    return 1;
+  else
+    return 0;
+}
+
+int __attribute__ ((noinline, noclone))
+intermediary (char *p)
+{
+  return test_alignment (p);
+}
+
+int
+main (int argc, char *argv[])
+{
+  ip_fw_fwd_addr.ip_dst.s_addr = 1;
+  return intermediary ((void *) &ip_fw_fwd_addr);
+}

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

I had to add a test that the analyzed expression is not an SSA name.
This has been approved by Rchi on IRC yesterday.  Thus, I have
committed the following as revision 175703 after successful run of c
and c++ testsuite on sparc64 (and a bootstrap and test with another
patch on x86_64-linux).

Thanks,

Martin


2011-06-30  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/49094
	* tree-sra.c (tree_non_mode_aligned_mem_p): New function.
	(build_accesses_from_assign): Use it.

	* testsuite/gcc.dg/tree-ssa/pr49094.c: New test.


Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -1050,6 +1050,26 @@ disqualify_ops_if_throwing_stmt (gimple
   return false;
 }
 
+/* Return true iff type of EXP is not sufficiently aligned.  */
+
+static bool
+tree_non_mode_aligned_mem_p (tree exp)
+{
+  enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
+  unsigned int align;
+
+  if (TREE_CODE (exp) == SSA_NAME
+      || mode == BLKmode
+      || !STRICT_ALIGNMENT)
+    return false;
+
+  align = get_object_alignment (exp, BIGGEST_ALIGNMENT);
+  if (GET_MODE_ALIGNMENT (mode) > align)
+    return true;
+
+  return false;
+}
+
 /* Scan expressions occuring in STMT, create access structures for all accesses
    to candidates for scalarization and remove those candidates which occur in
    statements or expressions that prevent them from being split apart.  Return
@@ -1074,7 +1094,10 @@ build_accesses_from_assign (gimple stmt)
   lacc = build_access_from_expr_1 (lhs, stmt, true);
 
   if (lacc)
-    lacc->grp_assignment_write = 1;
+    {
+      lacc->grp_assignment_write = 1;
+      lacc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (rhs);
+    }
 
   if (racc)
     {
@@ -1082,6 +1105,7 @@ build_accesses_from_assign (gimple stmt)
       if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt)
 	  && !is_gimple_reg_type (racc->type))
 	bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
+      racc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (lhs);
     }
 
   if (lacc && racc
Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options "-O" } */
+
+struct in_addr {
+	unsigned int s_addr;
+};
+
+struct ip {
+	unsigned char ip_p;
+	unsigned short ip_sum;
+	struct	in_addr ip_src,ip_dst;
+} __attribute__ ((aligned(1), packed));
+
+struct ip ip_fw_fwd_addr;
+
+int test_alignment( char *m )
+{
+  struct ip *ip = (struct ip *) m;
+  struct in_addr pkt_dst;
+  pkt_dst = ip->ip_dst ;
+  if( pkt_dst.s_addr == 0 )
+    return 1;
+  else
+    return 0;
+}
+
+int __attribute__ ((noinline, noclone))
+intermediary (char *p)
+{
+  return test_alignment (p);
+}
+
+int
+main (int argc, char *argv[])
+{
+  ip_fw_fwd_addr.ip_dst.s_addr = 1;
+  return intermediary ((void *) &ip_fw_fwd_addr);
+}

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

On Thu, Jun 30, 2011 at 03:39:55PM +0200, Martin Jambor wrote:
> Hi,
> 
> I had to add a test that the analyzed expression is not an SSA name.
> This has been approved by Rchi on IRC yesterday.  Thus, I have
> committed the following as revision 175703 after successful run of c
> and c++ testsuite on sparc64 (and a bootstrap and test with another
> patch on x86_64-linux).

I also tested fortran on sparc64 but missed a regression there
(gfortran.dg/pr25923.f90).  The problem is that *arg_1(D) is also
scrutinized, get_object_alignment returns 8 for it and that is
obviously not enough for SImode required alignment.

I assume such loads have to be aligned for the mode on strict aligned
targets and therefore they are OK.  The question is, is that true for
all MEM_REFs or only for those with zero offset?  Since the original
problem was that the expander expanded

MEM[(struct ip *)ip_3 + 7B].s_addr;

as if it was aligned, I suppose that MEM_REFs are generally fine and
we can avoid this issue by skipping them just like the SSA_NAMEs.  Is
my reasoning correct?

Thanks,

Martin


> 
> Thanks,
> 
> Martin
> 
> 
> 2011-06-30  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/49094
> 	* tree-sra.c (tree_non_mode_aligned_mem_p): New function.
> 	(build_accesses_from_assign): Use it.
> 
> 	* testsuite/gcc.dg/tree-ssa/pr49094.c: New test.
> 
> 
> Index: src/gcc/tree-sra.c
> ===================================================================
> --- src.orig/gcc/tree-sra.c
> +++ src/gcc/tree-sra.c
> @@ -1050,6 +1050,26 @@ disqualify_ops_if_throwing_stmt (gimple
>    return false;
>  }
>  
> +/* Return true iff type of EXP is not sufficiently aligned.  */
> +
> +static bool
> +tree_non_mode_aligned_mem_p (tree exp)
> +{
> +  enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
> +  unsigned int align;
> +
> +  if (TREE_CODE (exp) == SSA_NAME
> +      || mode == BLKmode
> +      || !STRICT_ALIGNMENT)
> +    return false;
> +
> +  align = get_object_alignment (exp, BIGGEST_ALIGNMENT);
> +  if (GET_MODE_ALIGNMENT (mode) > align)
> +    return true;
> +
> +  return false;
> +}
> +
>  /* Scan expressions occuring in STMT, create access structures for all accesses
>     to candidates for scalarization and remove those candidates which occur in
>     statements or expressions that prevent them from being split apart.  Return
> @@ -1074,7 +1094,10 @@ build_accesses_from_assign (gimple stmt)
>    lacc = build_access_from_expr_1 (lhs, stmt, true);
>  
>    if (lacc)
> -    lacc->grp_assignment_write = 1;
> +    {
> +      lacc->grp_assignment_write = 1;
> +      lacc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (rhs);
> +    }
>  
>    if (racc)
>      {
> @@ -1082,6 +1105,7 @@ build_accesses_from_assign (gimple stmt)
>        if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt)
>  	  && !is_gimple_reg_type (racc->type))
>  	bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
> +      racc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (lhs);
>      }
>  
>    if (lacc && racc
> Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
> @@ -0,0 +1,38 @@
> +/* { dg-do run } */
> +/* { dg-options "-O" } */
> +
> +struct in_addr {
> +	unsigned int s_addr;
> +};
> +
> +struct ip {
> +	unsigned char ip_p;
> +	unsigned short ip_sum;
> +	struct	in_addr ip_src,ip_dst;
> +} __attribute__ ((aligned(1), packed));
> +
> +struct ip ip_fw_fwd_addr;
> +
> +int test_alignment( char *m )
> +{
> +  struct ip *ip = (struct ip *) m;
> +  struct in_addr pkt_dst;
> +  pkt_dst = ip->ip_dst ;
> +  if( pkt_dst.s_addr == 0 )
> +    return 1;
> +  else
> +    return 0;
> +}
> +
> +int __attribute__ ((noinline, noclone))
> +intermediary (char *p)
> +{
> +  return test_alignment (p);
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  ip_fw_fwd_addr.ip_dst.s_addr = 1;
> +  return intermediary ((void *) &ip_fw_fwd_addr);
> +}

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4932 bytes --]

On Thu, 30 Jun 2011, Martin Jambor wrote:

> Hi,
> 
> On Thu, Jun 30, 2011 at 03:39:55PM +0200, Martin Jambor wrote:
> > Hi,
> > 
> > I had to add a test that the analyzed expression is not an SSA name.
> > This has been approved by Rchi on IRC yesterday.  Thus, I have
> > committed the following as revision 175703 after successful run of c
> > and c++ testsuite on sparc64 (and a bootstrap and test with another
> > patch on x86_64-linux).
> 
> I also tested fortran on sparc64 but missed a regression there
> (gfortran.dg/pr25923.f90).  The problem is that *arg_1(D) is also
> scrutinized, get_object_alignment returns 8 for it and that is
> obviously not enough for SImode required alignment.
> 
> I assume such loads have to be aligned for the mode on strict aligned
> targets and therefore they are OK.  The question is, is that true for
> all MEM_REFs or only for those with zero offset?  Since the original
> problem was that the expander expanded
> 
> MEM[(struct ip *)ip_3 + 7B].s_addr;
> 
> as if it was aligned, I suppose that MEM_REFs are generally fine and
> we can avoid this issue by skipping them just like the SSA_NAMEs.  Is
> my reasoning correct?

Not really.  This just highlights the issue that alignment on
strict-align targets is broken for any non-component-ref style
access.  As we happily fold component-refs into the MEM_REFs offset
the issue is now more appearant.

For MEM_REFs the alignment is supposed to be at least that of
TYPE_ALIGN (TREE_TYPE (mem-ref)), even though the expanders
would ignore that.

Richard.

> Thanks,
> 
> Martin
> 
> 
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2011-06-30  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	PR tree-optimization/49094
> > 	* tree-sra.c (tree_non_mode_aligned_mem_p): New function.
> > 	(build_accesses_from_assign): Use it.
> > 
> > 	* testsuite/gcc.dg/tree-ssa/pr49094.c: New test.
> > 
> > 
> > Index: src/gcc/tree-sra.c
> > ===================================================================
> > --- src.orig/gcc/tree-sra.c
> > +++ src/gcc/tree-sra.c
> > @@ -1050,6 +1050,26 @@ disqualify_ops_if_throwing_stmt (gimple
> >    return false;
> >  }
> >  
> > +/* Return true iff type of EXP is not sufficiently aligned.  */
> > +
> > +static bool
> > +tree_non_mode_aligned_mem_p (tree exp)
> > +{
> > +  enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
> > +  unsigned int align;
> > +
> > +  if (TREE_CODE (exp) == SSA_NAME
> > +      || mode == BLKmode
> > +      || !STRICT_ALIGNMENT)
> > +    return false;
> > +
> > +  align = get_object_alignment (exp, BIGGEST_ALIGNMENT);
> > +  if (GET_MODE_ALIGNMENT (mode) > align)
> > +    return true;
> > +
> > +  return false;
> > +}
> > +
> >  /* Scan expressions occuring in STMT, create access structures for all accesses
> >     to candidates for scalarization and remove those candidates which occur in
> >     statements or expressions that prevent them from being split apart.  Return
> > @@ -1074,7 +1094,10 @@ build_accesses_from_assign (gimple stmt)
> >    lacc = build_access_from_expr_1 (lhs, stmt, true);
> >  
> >    if (lacc)
> > -    lacc->grp_assignment_write = 1;
> > +    {
> > +      lacc->grp_assignment_write = 1;
> > +      lacc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (rhs);
> > +    }
> >  
> >    if (racc)
> >      {
> > @@ -1082,6 +1105,7 @@ build_accesses_from_assign (gimple stmt)
> >        if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt)
> >  	  && !is_gimple_reg_type (racc->type))
> >  	bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
> > +      racc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (lhs);
> >      }
> >  
> >    if (lacc && racc
> > Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
> > ===================================================================
> > --- /dev/null
> > +++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49094.c
> > @@ -0,0 +1,38 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-O" } */
> > +
> > +struct in_addr {
> > +	unsigned int s_addr;
> > +};
> > +
> > +struct ip {
> > +	unsigned char ip_p;
> > +	unsigned short ip_sum;
> > +	struct	in_addr ip_src,ip_dst;
> > +} __attribute__ ((aligned(1), packed));
> > +
> > +struct ip ip_fw_fwd_addr;
> > +
> > +int test_alignment( char *m )
> > +{
> > +  struct ip *ip = (struct ip *) m;
> > +  struct in_addr pkt_dst;
> > +  pkt_dst = ip->ip_dst ;
> > +  if( pkt_dst.s_addr == 0 )
> > +    return 1;
> > +  else
> > +    return 0;
> > +}
> > +
> > +int __attribute__ ((noinline, noclone))
> > +intermediary (char *p)
> > +{
> > +  return test_alignment (p);
> > +}
> > +
> > +int
> > +main (int argc, char *argv[])
> > +{
> > +  ip_fw_fwd_addr.ip_dst.s_addr = 1;
> > +  return intermediary ((void *) &ip_fw_fwd_addr);
> > +}
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

On Thu, Jun 30, 2011 at 06:36:01PM +0200, Martin Jambor wrote:
> Hi,
> 
> On Thu, Jun 30, 2011 at 03:39:55PM +0200, Martin Jambor wrote:
> > Hi,
> > 
> > I had to add a test that the analyzed expression is not an SSA name.
> > This has been approved by Rchi on IRC yesterday.  Thus, I have
> > committed the following as revision 175703 after successful run of c
> > and c++ testsuite on sparc64 (and a bootstrap and test with another
> > patch on x86_64-linux).
> 
> I also tested fortran on sparc64 but missed a regression there
> (gfortran.dg/pr25923.f90).  The problem is that *arg_1(D) is also
> scrutinized, get_object_alignment returns 8 for it and that is
> obviously not enough for SImode required alignment.
> 

The following patch resolves this issue, has been approved on IRC by
Richi and I have just committed it.

Thanks,

Martin


2011-07-01  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (tree_non_mode_aligned_mem_p): Also ignore MEM_REFs.

Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -1076,6 +1076,7 @@ tree_non_mode_aligned_mem_p (tree exp)
   unsigned int align;
 
   if (TREE_CODE (exp) == SSA_NAME
+      || TREE_CODE (exp) == MEM_REF
       || mode == BLKmode
       || !STRICT_ALIGNMENT)
     return false;

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Ulrich Weigand]

Martin Jambor wrote:

> I had to add a test that the analyzed expression is not an SSA name.
> This has been approved by Rchi on IRC yesterday.  Thus, I have
> committed the following as revision 175703 after successful run of c
> and c++ testsuite on sparc64 (and a bootstrap and test with another
> patch on x86_64-linux).
> 
> Thanks,
> 
> Martin
> 
> 
> 2011-06-30  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/49094
> 	* tree-sra.c (tree_non_mode_aligned_mem_p): New function.
> 	(build_accesses_from_assign): Use it.

This causes a regression on spu-elf:
FAIL: gcc.dg/tree-ssa/forwprop-5.c scan-tree-dump-times optimized "disappear" 0

The problem is that in this expression
  disappear = VIEW_CONVERT_EXPR<struct VecClass>(x_8);
the rhs is considered unaligned and blocks the SRA transformation.

The check you added for SSA_NAMEs doesn't hit, because the SSA_NAME is
encapsulated in a VIEW_CONVERT_EXPR.  When get_object_alignment is called,
the VIEW_CONVERT_EXPR is stripped off by get_inner_reference and the
SSA_NAME appears, but then get_object_alignment doesn't handle it
and just returns the default alignment of 8 bits.

Maybe get_object_alignment should itself handle SSA_NAMEs?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Richard Guenther]

On Tue, Jul 19, 2011 at 8:20 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Martin Jambor wrote:
>
>> I had to add a test that the analyzed expression is not an SSA name.
>> This has been approved by Rchi on IRC yesterday.  Thus, I have
>> committed the following as revision 175703 after successful run of c
>> and c++ testsuite on sparc64 (and a bootstrap and test with another
>> patch on x86_64-linux).
>>
>> Thanks,
>>
>> Martin
>>
>>
>> 2011-06-30  Martin Jambor  <mjambor@suse.cz>
>>
>>       PR tree-optimization/49094
>>       * tree-sra.c (tree_non_mode_aligned_mem_p): New function.
>>       (build_accesses_from_assign): Use it.
>
> This causes a regression on spu-elf:
> FAIL: gcc.dg/tree-ssa/forwprop-5.c scan-tree-dump-times optimized "disappear" 0
>
> The problem is that in this expression
>  disappear = VIEW_CONVERT_EXPR<struct VecClass>(x_8);
> the rhs is considered unaligned and blocks the SRA transformation.
>
> The check you added for SSA_NAMEs doesn't hit, because the SSA_NAME is
> encapsulated in a VIEW_CONVERT_EXPR.  When get_object_alignment is called,
> the VIEW_CONVERT_EXPR is stripped off by get_inner_reference and the
> SSA_NAME appears, but then get_object_alignment doesn't handle it
> and just returns the default alignment of 8 bits.
>
> Maybe get_object_alignment should itself handle SSA_NAMEs?

But what should it return for a rvalue?  There is no "alignment" here.  I think
SRA should avoid asking for rvalues.

Richard.

> Bye,
> Ulrich
>
> --
>  Dr. Ulrich Weigand
>  GNU Toolchain for Linux on System z and Cell BE
>  Ulrich.Weigand@de.ibm.com
>

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Ulrich Weigand]

Richard Guenther wrote:
> On Tue, Jul 19, 2011 at 8:20 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > The problem is that in this expression
> >   disappear = VIEW_CONVERT_EXPR<struct VecClass>(x_8);
> > the rhs is considered unaligned and blocks the SRA transformation.
> >
> > The check you added for SSA_NAMEs doesn't hit, because the SSA_NAME is
> > encapsulated in a VIEW_CONVERT_EXPR. When get_object_alignment is called,
> > the VIEW_CONVERT_EXPR is stripped off by get_inner_reference and the
> > SSA_NAME appears, but then get_object_alignment doesn't handle it
> > and just returns the default alignment of 8 bits.
> >
> > Maybe get_object_alignment should itself handle SSA_NAMEs?
> 
> But what should it return for a rvalue?  There is no "alignment" here.
> I think SRA should avoid asking for rvalues.

I must admit I do not fully understand what the SRA code is attempting
to achieve here ...  Could you elaborate on what you mean by "avoid
asking for rvalues"?  Should the SRA code never check the RHS of an
assignment for alignment, only the LHS?  Or should it classify the RHS
tree according to whether the access is rvalue or lvalue (how would
that work?)?

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Richard Guenther]

On Wed, 20 Jul 2011, Ulrich Weigand wrote:

> Richard Guenther wrote:
> > On Tue, Jul 19, 2011 at 8:20 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > > The problem is that in this expression
> > >   disappear = VIEW_CONVERT_EXPR<struct VecClass>(x_8);
> > > the rhs is considered unaligned and blocks the SRA transformation.
> > >
> > > The check you added for SSA_NAMEs doesn't hit, because the SSA_NAME is
> > > encapsulated in a VIEW_CONVERT_EXPR. When get_object_alignment is called,
> > > the VIEW_CONVERT_EXPR is stripped off by get_inner_reference and the
> > > SSA_NAME appears, but then get_object_alignment doesn't handle it
> > > and just returns the default alignment of 8 bits.
> > >
> > > Maybe get_object_alignment should itself handle SSA_NAMEs?
> > 
> > But what should it return for a rvalue?  There is no "alignment" here.
> > I think SRA should avoid asking for rvalues.
> 
> I must admit I do not fully understand what the SRA code is attempting
> to achieve here ...  Could you elaborate on what you mean by "avoid
> asking for rvalues"?  Should the SRA code never check the RHS of an
> assignment for alignment, only the LHS?  Or should it classify the RHS
> tree according to whether the access is rvalue or lvalue (how would
> that work?)?

Well, it should only ask for stores / loads.  I'm not sure what we'd
want to return as alignment for an rvalue - MAX_ALIGNMENT?  What should
we return for get_object_alignment of an INTEGER_CST for example?

Richard.

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

On Thu, Jul 21, 2011 at 10:34:35AM +0200, Richard Guenther wrote:
> On Wed, 20 Jul 2011, Ulrich Weigand wrote:
> 
> > Richard Guenther wrote:
> > > On Tue, Jul 19, 2011 at 8:20 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > > > The problem is that in this expression
> > > >   disappear = VIEW_CONVERT_EXPR<struct VecClass>(x_8);
> > > > the rhs is considered unaligned and blocks the SRA transformation.
> > > >
> > > > The check you added for SSA_NAMEs doesn't hit, because the SSA_NAME is
> > > > encapsulated in a VIEW_CONVERT_EXPR. When get_object_alignment is called,
> > > > the VIEW_CONVERT_EXPR is stripped off by get_inner_reference and the
> > > > SSA_NAME appears, but then get_object_alignment doesn't handle it
> > > > and just returns the default alignment of 8 bits.
> > > >
> > > > Maybe get_object_alignment should itself handle SSA_NAMEs?
> > > 
> > > But what should it return for a rvalue?  There is no "alignment" here.
> > > I think SRA should avoid asking for rvalues.
> > 
> > I must admit I do not fully understand what the SRA code is attempting
> > to achieve here ...  Could you elaborate on what you mean by "avoid
> > asking for rvalues"?  Should the SRA code never check the RHS of an
> > assignment for alignment, only the LHS?  Or should it classify the RHS
> > tree according to whether the access is rvalue or lvalue (how would
> > that work?)?
> 
> Well, it should only ask for stores / loads.  I'm not sure what we'd
> want to return as alignment for an rvalue - MAX_ALIGNMENT?  What should
> we return for get_object_alignment of an INTEGER_CST for example?
> 

Yeah, we certainly should not be examining alingment of invariants and
of conversions of ssa names in.  As far as rvalues in general are
concerned, I don't really know which gimple predicate that would be.
A comment suggests is_gimple_val but that does not return true for a
VIEW_CONVERT_EXPR of an SSA_NAME and would return true for aggregate
variables (which perhaps would not be a problem, however they do have
an alignment).

So at the moment I'd go for stripping all conversions from exp before
the first if in tree_non_mode_aligned_mem_p and adding
is_gimple_min_invariant to the condition.  Does that make sense?

(I'm quite puzzled why non-gimple_reg vectors and complex numbers are
not is_gimple_val while aggregate variables are and how that can be
useful but that is unrelated.)

Thanks,

Martin

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

On Thu, Jul 21, 2011 at 11:40:32AM +0200, Martin Jambor wrote:
> Hi,
> 
> On Thu, Jul 21, 2011 at 10:34:35AM +0200, Richard Guenther wrote:
> > On Wed, 20 Jul 2011, Ulrich Weigand wrote:
> > 
> > > Richard Guenther wrote:
> > > > On Tue, Jul 19, 2011 at 8:20 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > > > > The problem is that in this expression
> > > > >   disappear = VIEW_CONVERT_EXPR<struct VecClass>(x_8);
> > > > > the rhs is considered unaligned and blocks the SRA transformation.
> > > > >
> > > > > The check you added for SSA_NAMEs doesn't hit, because the SSA_NAME is
> > > > > encapsulated in a VIEW_CONVERT_EXPR. When get_object_alignment is called,
> > > > > the VIEW_CONVERT_EXPR is stripped off by get_inner_reference and the
> > > > > SSA_NAME appears, but then get_object_alignment doesn't handle it
> > > > > and just returns the default alignment of 8 bits.
> > > > >
> > > > > Maybe get_object_alignment should itself handle SSA_NAMEs?
> > > > 
> > > > But what should it return for a rvalue?  There is no "alignment" here.
> > > > I think SRA should avoid asking for rvalues.
> > > 
> > > I must admit I do not fully understand what the SRA code is attempting
> > > to achieve here ...  Could you elaborate on what you mean by "avoid
> > > asking for rvalues"?  Should the SRA code never check the RHS of an
> > > assignment for alignment, only the LHS?  Or should it classify the RHS
> > > tree according to whether the access is rvalue or lvalue (how would
> > > that work?)?
> > 
> > Well, it should only ask for stores / loads.  I'm not sure what we'd
> > want to return as alignment for an rvalue - MAX_ALIGNMENT?  What should
> > we return for get_object_alignment of an INTEGER_CST for example?
> > 
> 
> Yeah, we certainly should not be examining alingment of invariants and
> of conversions of ssa names in.  As far as rvalues in general are
> concerned, I don't really know which gimple predicate that would be.
> A comment suggests is_gimple_val but that does not return true for a
> VIEW_CONVERT_EXPR of an SSA_NAME and would return true for aggregate
> variables (which perhaps would not be a problem, however they do have
> an alignment).
> 
> So at the moment I'd go for stripping all conversions from exp before
> the first if in tree_non_mode_aligned_mem_p and adding
> is_gimple_min_invariant to the condition.  Does that make sense?


Like this?  Ulrich, can you please verify it works?  I have
bootstrapped this on x86_64 but there it obvioulsy works and my run of
compile/testsuite on compile farm sparc64 will take some time (plus,
the testcase you complained about passes there).

Thanks,

Martin


2011-07-25  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (tree_non_mode_aligned_mem_p): Strip conversions and
	return false for invariants.

Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -1075,9 +1075,14 @@ tree_non_mode_aligned_mem_p (tree exp)
   enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
   unsigned int align;
 
+  while (CONVERT_EXPR_P (exp)
+	 || TREE_CODE (exp) == VIEW_CONVERT_EXPR)
+    exp = TREE_OPERAND (exp, 0);
+
   if (TREE_CODE (exp) == SSA_NAME
       || TREE_CODE (exp) == MEM_REF
       || mode == BLKmode
+      || is_gimple_min_invariant (exp)
       || !STRICT_ALIGNMENT)
     return false;
 

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Ulrich Weigand]

Martin Jambor wrote:

> Like this?  Ulrich, can you please verify it works?  I have
> bootstrapped this on x86_64 but there it obvioulsy works and my run of
> compile/testsuite on compile farm sparc64 will take some time (plus,
> the testcase you complained about passes there).

Yes, this does fix the testcase that was failing on SPU.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Richard Guenther]

On Mon, Jul 25, 2011 at 7:52 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Thu, Jul 21, 2011 at 11:40:32AM +0200, Martin Jambor wrote:
>> Hi,
>>
>> On Thu, Jul 21, 2011 at 10:34:35AM +0200, Richard Guenther wrote:
>> > On Wed, 20 Jul 2011, Ulrich Weigand wrote:
>> >
>> > > Richard Guenther wrote:
>> > > > On Tue, Jul 19, 2011 at 8:20 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> > > > > The problem is that in this expression
>> > > > >   disappear = VIEW_CONVERT_EXPR<struct VecClass>(x_8);
>> > > > > the rhs is considered unaligned and blocks the SRA transformation.
>> > > > >
>> > > > > The check you added for SSA_NAMEs doesn't hit, because the SSA_NAME is
>> > > > > encapsulated in a VIEW_CONVERT_EXPR. When get_object_alignment is called,
>> > > > > the VIEW_CONVERT_EXPR is stripped off by get_inner_reference and the
>> > > > > SSA_NAME appears, but then get_object_alignment doesn't handle it
>> > > > > and just returns the default alignment of 8 bits.
>> > > > >
>> > > > > Maybe get_object_alignment should itself handle SSA_NAMEs?
>> > > >
>> > > > But what should it return for a rvalue?  There is no "alignment" here.
>> > > > I think SRA should avoid asking for rvalues.
>> > >
>> > > I must admit I do not fully understand what the SRA code is attempting
>> > > to achieve here ...  Could you elaborate on what you mean by "avoid
>> > > asking for rvalues"?  Should the SRA code never check the RHS of an
>> > > assignment for alignment, only the LHS?  Or should it classify the RHS
>> > > tree according to whether the access is rvalue or lvalue (how would
>> > > that work?)?
>> >
>> > Well, it should only ask for stores / loads.  I'm not sure what we'd
>> > want to return as alignment for an rvalue - MAX_ALIGNMENT?  What should
>> > we return for get_object_alignment of an INTEGER_CST for example?
>> >
>>
>> Yeah, we certainly should not be examining alingment of invariants and
>> of conversions of ssa names in.  As far as rvalues in general are
>> concerned, I don't really know which gimple predicate that would be.
>> A comment suggests is_gimple_val but that does not return true for a
>> VIEW_CONVERT_EXPR of an SSA_NAME and would return true for aggregate
>> variables (which perhaps would not be a problem, however they do have
>> an alignment).
>>
>> So at the moment I'd go for stripping all conversions from exp before
>> the first if in tree_non_mode_aligned_mem_p and adding
>> is_gimple_min_invariant to the condition.  Does that make sense?
>
>
> Like this?  Ulrich, can you please verify it works?  I have
> bootstrapped this on x86_64 but there it obvioulsy works and my run of
> compile/testsuite on compile farm sparc64 will take some time (plus,
> the testcase you complained about passes there).
>
> Thanks,
>
> Martin
>
>
> 2011-07-25  Martin Jambor  <mjambor@suse.cz>
>
>        * tree-sra.c (tree_non_mode_aligned_mem_p): Strip conversions and
>        return false for invariants.
>
> Index: src/gcc/tree-sra.c
> ===================================================================
> --- src.orig/gcc/tree-sra.c
> +++ src/gcc/tree-sra.c
> @@ -1075,9 +1075,14 @@ tree_non_mode_aligned_mem_p (tree exp)
>   enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
>   unsigned int align;
>
> +  while (CONVERT_EXPR_P (exp)

There can be no convert exprs here, and there can be at most one
VIEW_CONVERT_EXPR.

> +        || TREE_CODE (exp) == VIEW_CONVERT_EXPR)
> +    exp = TREE_OPERAND (exp, 0);
> +
>   if (TREE_CODE (exp) == SSA_NAME
>       || TREE_CODE (exp) == MEM_REF
>       || mode == BLKmode
> +      || is_gimple_min_invariant (exp)
>       || !STRICT_ALIGNMENT)
>     return false;

Otherwise ok.

Thanks,
Richard.

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

On Tue, Jul 26, 2011 at 09:39:02AM +0200, Richard Guenther wrote:
> On Mon, Jul 25, 2011 at 7:52 PM, Martin Jambor <mjambor@suse.cz> wrote:

...

> >
> > 2011-07-25  Martin Jambor  <mjambor@suse.cz>
> >
> >        * tree-sra.c (tree_non_mode_aligned_mem_p): Strip conversions and
> >        return false for invariants.
> >
> > Index: src/gcc/tree-sra.c
> > ===================================================================
> > --- src.orig/gcc/tree-sra.c
> > +++ src/gcc/tree-sra.c
> > @@ -1075,9 +1075,14 @@ tree_non_mode_aligned_mem_p (tree exp)
> >   enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
> >   unsigned int align;
> >
> > +  while (CONVERT_EXPR_P (exp)
> 
> There can be no convert exprs here, and there can be at most one
> VIEW_CONVERT_EXPR.
> 
> > +        || TREE_CODE (exp) == VIEW_CONVERT_EXPR)
> > +    exp = TREE_OPERAND (exp, 0);
> > +
> >   if (TREE_CODE (exp) == SSA_NAME
> >       || TREE_CODE (exp) == MEM_REF
> >       || mode == BLKmode
> > +      || is_gimple_min_invariant (exp)
> >       || !STRICT_ALIGNMENT)
> >     return false;
> 
> Otherwise ok.
> 

OK, this is what I have just committed as revision 176797 after
re-testing.

Thanks,

Martin


2011-07-26  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (tree_non_mode_aligned_mem_p): Strip conversions and
	return false for invariants.

Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -1075,9 +1075,13 @@ tree_non_mode_aligned_mem_p (tree exp)
   enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
   unsigned int align;
 
+  if (TREE_CODE (exp) == VIEW_CONVERT_EXPR)
+    exp = TREE_OPERAND (exp, 0);
+
   if (TREE_CODE (exp) == SSA_NAME
       || TREE_CODE (exp) == MEM_REF
       || mode == BLKmode
+      || is_gimple_min_invariant (exp)
       || !STRICT_ALIGNMENT)
     return false;
 

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Ulrich Weigand]

Martin Jambor wrote:

> OK, this is what I have just committed as revision 176797 after
> re-testing.

Thanks, this has fixed the forwprop-5.c regression on spu-elf on mainline.

I'm seeing the same failure on the 4.6 branch -- would this patch also be
appropriate there?

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Martin Jambor]

Hi,

On Wed, Jul 27, 2011 at 02:34:59PM +0200, Ulrich Weigand wrote:
> Martin Jambor wrote:
> 
> > OK, this is what I have just committed as revision 176797 after
> > re-testing.
> 
> Thanks, this has fixed the forwprop-5.c regression on spu-elf on mainline.
> 
> I'm seeing the same failure on the 4.6 branch -- would this patch also be
> appropriate there?
> 

You're right, it should be applied to the 4.6 branch too.  Since you
have the setup to thest it, can you do it please?  Otherwise I'll do
it in a few days.

Thanks,

Martin

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Ulrich Weigand]

Martin Jambor wrote:
> On Wed, Jul 27, 2011 at 02:34:59PM +0200, Ulrich Weigand wrote:
> > I'm seeing the same failure on the 4.6 branch -- would this patch also be
> > appropriate there?
> 
> You're right, it should be applied to the 4.6 branch too.  Since you
> have the setup to thest it, can you do it please?  Otherwise I'll do
> it in a few days.

Sure; I've verified that the patch does fix the test case regression
on the branch as well.  A full regression test is still running ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Ulrich Weigand]

Martin Jambor wrote:
> On Wed, Jul 27, 2011 at 02:34:59PM +0200, Ulrich Weigand wrote:
> > Martin Jambor wrote:
> > 
> > > OK, this is what I have just committed as revision 176797 after
> > > re-testing.
> > 
> > Thanks, this has fixed the forwprop-5.c regression on spu-elf on mainline.
> > 
> > I'm seeing the same failure on the 4.6 branch -- would this patch also be
> > appropriate there?
> > 
> 
> You're right, it should be applied to the 4.6 branch too.  Since you
> have the setup to thest it, can you do it please?  Otherwise I'll do
> it in a few days.

Full test on spu-elf has now completed.  In addition to the forwprop-5.c
regression, the patch also fixes this regression (see PR 49545):
FAIL: g++.dg/tree-ssa/fwprop-align.C scan-tree-dump-times forwprop2 "& 1" 0

No new regressions.

OK for the branch?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, PR 49094] Refrain from creating misaligned accesses in SRA

[Richard Guenther]

On Wed, Jul 27, 2011 at 9:23 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Martin Jambor wrote:
>> On Wed, Jul 27, 2011 at 02:34:59PM +0200, Ulrich Weigand wrote:
>> > Martin Jambor wrote:
>> >
>> > > OK, this is what I have just committed as revision 176797 after
>> > > re-testing.
>> >
>> > Thanks, this has fixed the forwprop-5.c regression on spu-elf on mainline.
>> >
>> > I'm seeing the same failure on the 4.6 branch -- would this patch also be
>> > appropriate there?
>> >
>>
>> You're right, it should be applied to the 4.6 branch too.  Since you
>> have the setup to thest it, can you do it please?  Otherwise I'll do
>> it in a few days.
>
> Full test on spu-elf has now completed.  In addition to the forwprop-5.c
> regression, the patch also fixes this regression (see PR 49545):
> FAIL: g++.dg/tree-ssa/fwprop-align.C scan-tree-dump-times forwprop2 "& 1" 0
>
> No new regressions.
>
> OK for the branch?

Ok.

Thanks,
Richard.

> Bye,
> Ulrich
>
> --
>  Dr. Ulrich Weigand
>  GNU Toolchain for Linux on System z and Cell BE
>  Ulrich.Weigand@de.ibm.com
>