New SPU failures (Re: [PATCH, PR 49923] Check for misaligned accesses before doing SRA)

[Richard Guenther]

On Fri, 9 Sep 2011, Martin Jambor wrote:

> Hi,
> 
> On Wed, Aug 10, 2011 at 04:29:40PM +0200, Richard Guenther wrote:
> > On Wed, 10 Aug 2011, Ulrich Weigand wrote:
> > 
> 
> ...
> 
> > > 
> > > FAIL: gcc.dg/ipa/ipa-sra-2.c scan-tree-dump eipa_sra "About to replace expr cow_.*D.->red with \\*ISRA"
> > > FAIL: gcc.dg/ipa/ipa-sra-2.c scan-tree-dump eipa_sra "About to replace expr cow_.*D.->green with ISRA"
> > > FAIL: gcc.dg/ipa/ipa-sra-2.c scan-tree-dump eipa_sra "About to replace expr calf_.*D.->red with \\*ISRA"
> > > FAIL: gcc.dg/ipa/ipa-sra-2.c scan-tree-dump eipa_sra "About to replace expr calf_.*D.->green with ISRA"
> > > FAIL: gcc.dg/ipa/ipa-sra-6.c scan-tree-dump-times eipa_sra "foo " 1
> 
> This has recvently been reported as PR 50052.
> 
> ...
> 
> > > 
> > > I must admin I continue to be confused about exactly what it is that
> > > tree_non_mode_aligned_mem_p is supposed to be testing for.  We have:
> > > 
> > >   if (TREE_CODE (exp) == SSA_NAME
> > >       || TREE_CODE (exp) == MEM_REF
> > >       || mode == BLKmode
> > >       || is_gimple_min_invariant (exp)
> > >       || !STRICT_ALIGNMENT)
> > >     return false;
> > > 
> > > So if we passed in the plain MEM_REF, this would be considered no problem.
> > > The COMPONENT_REF does not add any additional misalignment, so one would
> > > hope that this also shouldn't be a problem.
> > > 
> > > However, just because there *is* a COMPONENT_REF around it, we suddenly
> > > realize the fact that don't have points-to information for the MEM_REF
> > > and therefore consider *it* (and consequently the whole COMPONENT_REF)
> > > to be potentially misaligned ...
> > 
> > Yep.  That's because we are totally confused about alignment :/
> > 
> > Martin, what we need to do is get expands idea of what alignment
> > it woudl assume for a handled_component_ref and compare that
> > to what it would say if we re-materialize the mem as a MEM_REF.
> > 
> > Unfortunately there isn't a function that you can use to mimic
> > expands behavior (that of the normal_inner_ref: case), the closest
> > would be to look for TYPE_PACKED or TYPE_ALIGN in addition to
> > what get_object_alignment gives us.
> > 
> > Thus something like
> > 
> >   align = get_object_alignment (exp);
> >   if (!TYPE_PACKED (TREE_TYPE (exp))
> >       && (TREE_CODE (exp) != COMPONENT_REF
> >           || !DECL_PACKED (TREE_OPERAND (exp, 1))))
> >     align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), align);
> >   if (GET_MODE_ALIGNMENT (mode) > align)
> >     return true;
> > 
> 
> This fixed the failing testcase ipa-sra-2.c but it creates a
> misaligned access for the testcase below so I changed it to:
> 
>   align = get_object_alignment (exp);
>   if (!TYPE_PACKED (TREE_TYPE (exp)))
>     {
>       bool encountered_packed = false;
>       tree t = exp;
> 
>       while (TREE_CODE (t) == COMPONENT_REF)

that should be handled_component_p (t) then to catch p->a.b[i].c
with packed a.

But the normal_inner_ref: case doesn't suggest that we need to dive
into handled-components at all ... but it uses DECL_MODE of
an outermost COMPONENT_REF instead of TYPE_MODE.  Maybe the easiest
would be to simply call get_inner_reference like the normal_inner_ref
case does to obtain the mode, perform the packed_p check and
decide based on that (supposed that the mode difference is what
makes the new testcase fail).  Also look for the predicates that
guard the extract_bit_field call ... (yeah, I know ...)

>         if (DECL_PACKED (TREE_OPERAND (t, 1)))
>           {
>             encountered_packed = true;
>             break;
>           }
>         else
>           t = TREE_OPERAND (t, 0);
> 
>       if (!encountered_packed)
>         align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), align);
>     }
>   if (GET_MODE_ALIGNMENT (mode) > align)
>     return true;
> 
> I'm currently bootstrapping this on a sparc64 on the compile farm.  Is
> the patch OK if it passes (it has passed bootstrap and testing on
> x86_64-linux but that is no surprise)?
> 
> Thanks,
> 
> Martin
> 
> 
> 2011-09-08  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/50052
> 	* tree-sra.c (tree_non_mode_aligned_mem_p): Look at TYPE_ALIGN if
> 	there is no packed field decl in exp.
> 
> 	* gcc.dg/tree-ssa/pr49094-2.c: New test.
> 
> Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49094-2.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49094-2.c
> @@ -0,0 +1,44 @@
> +/* { dg-do run } */
> +/* { dg-options "-O" } */
> +
> +struct in_addr {
> +  unsigned int s_addr;
> +};
> +
> +struct ip {
> +  struct in_addr ip_src,ip_dst;
> +  unsigned char ip_p;
> +  unsigned short ip_sum;
> +};
> +
> +struct ip2 {
> +  unsigned char blah;
> +  unsigned short sheep;
> +  struct ip ip;
> +} __attribute__ ((aligned(1), packed));
> +
> +struct ip2 ip_fw_fwd_addr;
> +
> +int test_alignment( char *m )
> +{
> +  struct ip2 *ip = (struct ip2 *) m;
> +  struct in_addr pkt_dst;
> +  pkt_dst = ip->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.ip_dst.s_addr = 1;
> +  return intermediary ((void *) &ip_fw_fwd_addr);
> +}
> Index: src/gcc/tree-sra.c
> ===================================================================
> --- src.orig/gcc/tree-sra.c
> +++ src/gcc/tree-sra.c
> @@ -1086,6 +1086,23 @@ tree_non_mode_aligned_mem_p (tree exp)
>      return false;
>  
>    align = get_object_alignment (exp);
> +  if (!TYPE_PACKED (TREE_TYPE (exp)))
> +    {
> +      bool encountered_packed = false;
> +      tree t = exp;
> +
> +      while (TREE_CODE (t) == COMPONENT_REF)
> +        if (DECL_PACKED (TREE_OPERAND (t, 1)))
> +          {
> +            encountered_packed = true;
> +            break;
> +          }
> +        else
> +          t = TREE_OPERAND (t, 0);
> +
> +      if (!encountered_packed)
> +        align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), align);
> +    }
>    if (GET_MODE_ALIGNMENT (mode) > align)
>      return true;
>  
> 
> 

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