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

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.

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);
> +}
> 
>