| Message ID | 20110909124220.GA30519@virgil.arch.suse.de |
|---|---|
| State | New |
| Headers | show |
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; > > >
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; > > > > > > > >
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;