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 <[email protected]>
> >
> > 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 <[email protected]>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
