[PATCH, PR 49923] Check for misaligned accesses before doing SRA

[PATCH, PR 49923] Check for misaligned accesses before doing SRA

[Martin Jambor]

Hi,

the patch below fixes PR 49923 by checking for misaligned accesses
before doing IPA-SRA (on strict alignment targets).  I have checked it
fixes the issue on compile farm sparc64 and I also included this in a
bootstrap and testsuite run on an x86_64-linux just to double check.

OK for trunk and the 4.6 branch?

Thanks,

Martin


2011-08-04  Martin Jambor  <mjambor@suse.cz>

	PR middle-end/49923
	* tree-sra.c (access_precludes_ipa_sra_p): Also check access
	memory alignment.

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


Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -3688,6 +3688,9 @@ access_precludes_ipa_sra_p (struct acces
 	  || gimple_code (access->stmt) == GIMPLE_ASM))
     return true;
 
+  if (tree_non_mode_aligned_mem_p (access->expr))
+    return true;
+
   return false;
 }
 
Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49923.c
===================================================================
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49923.c
@@ -0,0 +1,55 @@
+/* { dg-do run } */
+/* { dg-options "-O3" } */
+
+#define PACKED __attribute__(( packed ))
+
+struct PACKED aostk_point_u8 {
+  unsigned char x;
+  unsigned char y;
+};
+
+struct PACKED aostk_size_u8 {
+  unsigned char width;
+  unsigned char height;
+};
+
+struct PACKED aostk_glyph {
+	unsigned short i;
+	struct aostk_size_u8 size;
+	char top;
+	struct aostk_point_u8 advance;
+	unsigned char pitch;
+	unsigned char* data;
+	char left;
+};
+
+
+struct PACKED aostk_font {
+	unsigned short numglyphs;
+	unsigned char height;
+	struct aostk_glyph* glyphs;
+};
+
+struct aostk_font glob_font;
+
+static struct aostk_glyph* aostk_get_glyph(struct aostk_font* f, unsigned int c) {
+	return f->glyphs;
+}
+
+int aostk_font_strwidth(struct aostk_font* font, const char* str) {
+	struct aostk_glyph* g = aostk_get_glyph(font, 0);
+	return (g != 0);
+}
+
+struct aostk_font*
+__attribute__ ((noinline, noclone))
+get_some_font (void)
+{
+  return &glob_font;
+}
+
+int main (int argc, char *argv[])
+{
+  return (int) aostk_font_strwidth (get_some_font (), "sth");
+  
+}

Re: [PATCH, PR 49923] Check for misaligned accesses before doing SRA

[Richard Guenther]

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

On Fri, 5 Aug 2011, Martin Jambor wrote:

> Hi,
> 
> the patch below fixes PR 49923 by checking for misaligned accesses
> before doing IPA-SRA (on strict alignment targets).  I have checked it
> fixes the issue on compile farm sparc64 and I also included this in a
> bootstrap and testsuite run on an x86_64-linux just to double check.
> 
> OK for trunk and the 4.6 branch?

Ok for now.

I think we need to move this to generic middle-end code and also
do something about partly strict-alignment targets such as x86_64.
Iff expansion would treat expr as if it had non-natural alignment
then when building a MEM_REF replacement with non-BLKmode we have
to use a properly aligned variant type for it.

I think we can trick FRE/PRE to run into exactly the same situation.

I'll put it on my TODO.

Richard.

> Thanks,
> 
> Martin
> 
> 
> 2011-08-04  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/49923
> 	* tree-sra.c (access_precludes_ipa_sra_p): Also check access
> 	memory alignment.
> 
> 	* testsuite/gcc.dg/tree-ssa/pr49923.c: New test.
> 
> 
> Index: src/gcc/tree-sra.c
> ===================================================================
> --- src.orig/gcc/tree-sra.c
> +++ src/gcc/tree-sra.c
> @@ -3688,6 +3688,9 @@ access_precludes_ipa_sra_p (struct acces
>  	  || gimple_code (access->stmt) == GIMPLE_ASM))
>      return true;
>  
> +  if (tree_non_mode_aligned_mem_p (access->expr))
> +    return true;
> +
>    return false;
>  }
>  
> Index: src/gcc/testsuite/gcc.dg/tree-ssa/pr49923.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.dg/tree-ssa/pr49923.c
> @@ -0,0 +1,55 @@
> +/* { dg-do run } */
> +/* { dg-options "-O3" } */
> +
> +#define PACKED __attribute__(( packed ))
> +
> +struct PACKED aostk_point_u8 {
> +  unsigned char x;
> +  unsigned char y;
> +};
> +
> +struct PACKED aostk_size_u8 {
> +  unsigned char width;
> +  unsigned char height;
> +};
> +
> +struct PACKED aostk_glyph {
> +	unsigned short i;
> +	struct aostk_size_u8 size;
> +	char top;
> +	struct aostk_point_u8 advance;
> +	unsigned char pitch;
> +	unsigned char* data;
> +	char left;
> +};
> +
> +
> +struct PACKED aostk_font {
> +	unsigned short numglyphs;
> +	unsigned char height;
> +	struct aostk_glyph* glyphs;
> +};
> +
> +struct aostk_font glob_font;
> +
> +static struct aostk_glyph* aostk_get_glyph(struct aostk_font* f, unsigned int c) {
> +	return f->glyphs;
> +}
> +
> +int aostk_font_strwidth(struct aostk_font* font, const char* str) {
> +	struct aostk_glyph* g = aostk_get_glyph(font, 0);
> +	return (g != 0);
> +}
> +
> +struct aostk_font*
> +__attribute__ ((noinline, noclone))
> +get_some_font (void)
> +{
> +  return &glob_font;
> +}
> +
> +int main (int argc, char *argv[])
> +{
> +  return (int) aostk_font_strwidth (get_some_font (), "sth");
> +  
> +}
> 
> 

-- 
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

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

[Ulrich Weigand]

Richard Guenther wrote:
> On Fri, 5 Aug 2011, Martin Jambor wrote:
> > the patch below fixes PR 49923 by checking for misaligned accesses
> > before doing IPA-SRA (on strict alignment targets).  I have checked it
> > fixes the issue on compile farm sparc64 and I also included this in a
> > bootstrap and testsuite run on an x86_64-linux just to double check.
> > 
> > OK for trunk and the 4.6 branch?
> 
> Ok for now.
> 
> I think we need to move this to generic middle-end code and also
> do something about partly strict-alignment targets such as x86_64.
> Iff expansion would treat expr as if it had non-natural alignment
> then when building a MEM_REF replacement with non-BLKmode we have
> to use a properly aligned variant type for it.
> 
> I think we can trick FRE/PRE to run into exactly the same situation.
> 
> I'll put it on my TODO.

This caused new failures on spu-elf:

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

Looking at the last case, tree_non_mode_aligned_mem_p gets called with
expressions like those:

 <component_ref 0xf6f4da40
    type <pointer_type 0xf6f602a0
        type <record_type 0xf6f60180 bovid sizes-gimplified type_0 BLK
            size <integer_cst 0xf6f4d680 constant 96>
            unit size <integer_cst 0xf6f4d6c0 constant 12>
            align 32 symtab 0 alias set 2 canonical type 0xf6f60180 fields <field_decl 0xf6f601e0 a> context <translation_unit_decl 0xf6ec10a0 D.2004>
            pointer_to_this <pointer_type 0xf6f602a0> chain <type_decl 0xf6ec1030 D.1991>>
        sizes-gimplified public unsigned SI
        size <integer_cst 0xf6e10580 constant 32>
        unit size <integer_cst 0xf6e105a0 constant 4>
        align 32 symtab 0 alias set 5 canonical type 0xf6f602a0>
   
    arg 0 <mem_ref 0xf6f4da60 type <record_type 0xf6f60180 bovid>
       
        arg 0 <ssa_name 0xf6f0ccf0 type <pointer_type 0xf6f602a0>
            visited var <parm_decl 0xf6f70000 cow>def_stmt GIMPLE_NOP

            version 3
            ptr-info 0xf6e760d0>
        arg 1 <integer_cst 0xf6f4d7a0 constant 0>
        /home/uweigand/fsf/gcc-head/gcc/testsuite/gcc.dg/ipa/ipa-sra-6.c:16:10>
    arg 1 <field_decl 0xf6f60300 next type <pointer_type 0xf6f602a0>
        unsigned SI file /home/uweigand/fsf/gcc-head/gcc/testsuite/gcc.dg/ipa/ipa-sra-6.c line 8 col 17 size <integer_cst 0xf6e10580 32> unit size <integer_cst 0xf6e105a0 4>
        align 32 offset_align 128
        offset <integer_cst 0xf6e105c0 constant 0>
        bit offset <integer_cst 0xf6e10620 constant 64> context <record_type 0xf6f60180 bovid>>
    /home/uweigand/fsf/gcc-head/gcc/testsuite/gcc.dg/ipa/ipa-sra-6.c:16:10>

Now, the component reference as such doesn't introduce any misalignment;
there are no attribute-aligned in place anywhere.

However, the ptr-info of the "cow" parameter is set up to assume nothing
about alignment.  Therefore, get_object_alignment returns 8, and
tree_non_mode_aligned_mem_p then of course fails.

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

Bye,
Ulrich

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

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

[Richard Guenther]

On Wed, 10 Aug 2011, Ulrich Weigand wrote:

> Richard Guenther wrote:
> > On Fri, 5 Aug 2011, Martin Jambor wrote:
> > > the patch below fixes PR 49923 by checking for misaligned accesses
> > > before doing IPA-SRA (on strict alignment targets).  I have checked it
> > > fixes the issue on compile farm sparc64 and I also included this in a
> > > bootstrap and testsuite run on an x86_64-linux just to double check.
> > > 
> > > OK for trunk and the 4.6 branch?
> > 
> > Ok for now.
> > 
> > I think we need to move this to generic middle-end code and also
> > do something about partly strict-alignment targets such as x86_64.
> > Iff expansion would treat expr as if it had non-natural alignment
> > then when building a MEM_REF replacement with non-BLKmode we have
> > to use a properly aligned variant type for it.
> > 
> > I think we can trick FRE/PRE to run into exactly the same situation.
> > 
> > I'll put it on my TODO.
> 
> This caused new failures on spu-elf:
> 
> 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
> 
> Looking at the last case, tree_non_mode_aligned_mem_p gets called with
> expressions like those:
> 
>  <component_ref 0xf6f4da40
>     type <pointer_type 0xf6f602a0
>         type <record_type 0xf6f60180 bovid sizes-gimplified type_0 BLK
>             size <integer_cst 0xf6f4d680 constant 96>
>             unit size <integer_cst 0xf6f4d6c0 constant 12>
>             align 32 symtab 0 alias set 2 canonical type 0xf6f60180 fields <field_decl 0xf6f601e0 a> context <translation_unit_decl 0xf6ec10a0 D.2004>
>             pointer_to_this <pointer_type 0xf6f602a0> chain <type_decl 0xf6ec1030 D.1991>>
>         sizes-gimplified public unsigned SI
>         size <integer_cst 0xf6e10580 constant 32>
>         unit size <integer_cst 0xf6e105a0 constant 4>
>         align 32 symtab 0 alias set 5 canonical type 0xf6f602a0>
>    
>     arg 0 <mem_ref 0xf6f4da60 type <record_type 0xf6f60180 bovid>
>        
>         arg 0 <ssa_name 0xf6f0ccf0 type <pointer_type 0xf6f602a0>
>             visited var <parm_decl 0xf6f70000 cow>def_stmt GIMPLE_NOP
> 
>             version 3
>             ptr-info 0xf6e760d0>
>         arg 1 <integer_cst 0xf6f4d7a0 constant 0>
>         /home/uweigand/fsf/gcc-head/gcc/testsuite/gcc.dg/ipa/ipa-sra-6.c:16:10>
>     arg 1 <field_decl 0xf6f60300 next type <pointer_type 0xf6f602a0>
>         unsigned SI file /home/uweigand/fsf/gcc-head/gcc/testsuite/gcc.dg/ipa/ipa-sra-6.c line 8 col 17 size <integer_cst 0xf6e10580 32> unit size <integer_cst 0xf6e105a0 4>
>         align 32 offset_align 128
>         offset <integer_cst 0xf6e105c0 constant 0>
>         bit offset <integer_cst 0xf6e10620 constant 64> context <record_type 0xf6f60180 bovid>>
>     /home/uweigand/fsf/gcc-head/gcc/testsuite/gcc.dg/ipa/ipa-sra-6.c:16:10>
> 
> Now, the component reference as such doesn't introduce any misalignment;
> there are no attribute-aligned in place anywhere.
> 
> However, the ptr-info of the "cow" parameter is set up to assume nothing
> about alignment.  Therefore, get_object_alignment returns 8, and
> tree_non_mode_aligned_mem_p then of course fails.
> 
> 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;

but really the twisted maze of how we compute whether something
is misaligned during expansion should be cleaned up / factored
somehow, including eventually fixing misaligned indirect refs
for STRICT_ALIGNMENT targets ...

Richard.

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

[Martin Jambor]

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

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

[Richard Guenther]

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

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

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

[Richard Guenther]

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

On Fri, 9 Sep 2011, Richard Guenther wrote:

> 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 ...)

Btw, it would be nice to try to produce a testcase for x86 by
using SSE vector types and make sure we do / do not expand to
unaligned moves (thus, both check for correctness and optimality).

Richard.

> >         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