On Thu, 12 Apr 2012, Martin Jambor wrote:
> Hi,
>
> On Wed, Apr 04, 2012 at 04:42:05PM +0200, Richard Guenther wrote:
> > On Wed, 4 Apr 2012, Martin Jambor wrote:
> >
> > > Hi everyone, especially Richi and Eric,
> > >
> > > I'd like to know what is your attitude to changing SRA's
> > > build_ref_for_model to what it once looked like, so that it produces
> > > COMPONENT_REFs only for bit-fields. The non-bit field handling was
> > > added in order to work-around problems when expanding non-aligned
> > > MEM_REFs on strict-alignment targets but that should not be a problem
> > > now and my experiments confirm that. Last week I successfully
> > > bootstrapped and tested this patch on sparc64-linux (with the
> > > temporary MEM_EXPR patch, not including Java), ia64-linux (without
> > > Ada), x86_64-linux, i686-linux and tested it on hppa-linux (only C and
> > > C++).
>
> ...
>
> > >
> > > 2012-03-20 Martin Jambor <mjam...@suse.cz>
> > >
> > > * tree-sra.c (build_ref_for_model): Create COMPONENT_REFs only for
> > > bit-fields.
> > >
> > > Index: src/gcc/tree-sra.c
> > > ===================================================================
> > > *** src.orig/gcc/tree-sra.c
> > > --- src/gcc/tree-sra.c
> > > *************** build_ref_for_offset (location_t loc, tr
> > > *** 1489,1558 ****
> > > return fold_build2_loc (loc, MEM_REF, exp_type, base, off);
> > > }
> > >
> > > - DEF_VEC_ALLOC_P_STACK (tree);
> > > - #define VEC_tree_stack_alloc(alloc) VEC_stack_alloc (tree, alloc)
> > > -
> > > /* Construct a memory reference to a part of an aggregate BASE at the
> > > given
> > > ! OFFSET and of the type of MODEL. In case this is a chain of
> > > references
> > > ! to component, the function will replicate the chain of
> > > COMPONENT_REFs of
> > > ! the expression of MODEL to access it. GSI and INSERT_AFTER have the
> > > same
> > > ! meaning as in build_ref_for_offset. */
> > >
> > > static tree
> > > build_ref_for_model (location_t loc, tree base, HOST_WIDE_INT offset,
> > > struct access *model, gimple_stmt_iterator *gsi,
> > > bool insert_after)
> > > {
> > > ! tree type = model->type, t;
> > > ! VEC(tree,stack) *cr_stack = NULL;
> > > !
> > > ! if (TREE_CODE (model->expr) == COMPONENT_REF)
> > > {
> > > ! tree expr = model->expr;
> > > !
> > > ! /* Create a stack of the COMPONENT_REFs so later we can walk them
> > > in
> > > ! order from inner to outer. */
> > > ! cr_stack = VEC_alloc (tree, stack, 6);
> > > !
> > > ! do {
> > > ! tree field = TREE_OPERAND (expr, 1);
> > > ! tree cr_offset = component_ref_field_offset (expr);
> > > ! HOST_WIDE_INT bit_pos
> > > ! = tree_low_cst (cr_offset, 1) * BITS_PER_UNIT
> > > ! + TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field));
> > >
> > > ! /* We can be called with a model different from the one
> > > associated
> > > ! with BASE so we need to avoid going up the chain too far. */
> > > ! if (offset - bit_pos < 0)
> > > ! break;
> > > !
> > > ! offset -= bit_pos;
> > > ! VEC_safe_push (tree, stack, cr_stack, expr);
> > > !
> > > ! expr = TREE_OPERAND (expr, 0);
> > > ! type = TREE_TYPE (expr);
> > > ! } while (TREE_CODE (expr) == COMPONENT_REF);
> > > }
> > > !
> > > ! t = build_ref_for_offset (loc, base, offset, type, gsi, insert_after);
> > > !
> > > ! if (TREE_CODE (model->expr) == COMPONENT_REF)
> > > ! {
> > > ! unsigned i;
> > > ! tree expr;
> > > !
> > > ! /* Now replicate the chain of COMPONENT_REFs from inner to outer.
> > > */
> > > ! FOR_EACH_VEC_ELT_REVERSE (tree, cr_stack, i, expr)
> > > ! {
> > > ! tree field = TREE_OPERAND (expr, 1);
> > > ! t = fold_build3_loc (loc, COMPONENT_REF, TREE_TYPE (field),
> > > t, field,
> > > ! TREE_OPERAND (expr, 2));
> > > ! }
> > > !
> > > ! VEC_free (tree, stack, cr_stack);
> > > ! }
> > > !
> > > ! return t;
> > > }
> > >
> > > /* Construct a memory reference consisting of component_refs and
> > > array_refs to
> > > --- 1489,1520 ----
> > > return fold_build2_loc (loc, MEM_REF, exp_type, base, off);
> > > }
> > >
> > > /* Construct a memory reference to a part of an aggregate BASE at the
> > > given
> > > ! OFFSET and of the same type as MODEL. In case this is a reference
> > > to a
> > > ! bit-field, the function will replicate the last component_ref of
> > > model's
> > > ! expr to access it. GSI and INSERT_AFTER have the same meaning as in
> > > ! build_ref_for_offset. */
> > >
> > > static tree
> > > build_ref_for_model (location_t loc, tree base, HOST_WIDE_INT offset,
> > > struct access *model, gimple_stmt_iterator *gsi,
> > > bool insert_after)
> > > {
> > > ! if (TREE_CODE (model->expr) == COMPONENT_REF
> > > ! && DECL_BIT_FIELD (TREE_OPERAND (model->expr, 1)))
> >
> > I think you need to test DECL_BIT_FIELD_TYPE here
>
> I have no problems changing that but in between revisions 164280 and
> 166730 this test worked fine. What exactly is the difference?
>
> >
> > > {
> > > ! /* This access represents a bit-field. */
> > > ! tree t, exp_type;
> > >
> > > ! offset -= int_bit_position (TREE_OPERAND (model->expr, 1));
> >
> > I'm not sure that offset is now byte-aligned for all the funny Ada
> > bit-layouted records. But maybe we don't even try to SRA those?
>
> Aggregates containing aggregate fields which start at position
> straddling byte boundary are not considered candidates for SRA.
>
> >
> > > ! exp_type = TREE_TYPE (TREE_OPERAND (model->expr, 0));
> > > ! t = build_ref_for_offset (loc, base, offset, exp_type, gsi,
> > > insert_after);
> > > ! return fold_build3_loc (loc, COMPONENT_REF, model->type, t,
> > > ! TREE_OPERAND (model->expr, 1), NULL_TREE);
> > > }
> > > ! else
> > > ! return build_ref_for_offset (loc, base, offset, model->type,
> > > ! gsi, insert_after);
> > > }
> > >
> > > /* Construct a memory reference consisting of component_refs and
> > > array_refs to
> >
> > Otherwise I'm all for doing this change. I'd even go one step further
> > and lower bitfield accesses here in SRA - if the FIELD_DECL of the
> > bitfield COMPONENT_REF has a DECL_BIT_FIELD_REPRESENTATIVE access
> > that and extract the requested bits via a BIT_FIELD_REF on the RHS
> > (or do a RMW cycle for stores).
> >
> > That would have been my first place to "lower" bitfield accesses anyway,
> > and maybe get rid of some restrictions of SRA that way.
>
> I was also considering that when I saw the representatives patch but
> then I wondered why we'd only lower bit-field accesses to
> non-addressable structures... but I admit I have not really thought it
> through, I guess that can also present some problems.
Well, just because SRA only touches non-addressable structures ...
(that is, if SRA wants to touch the structure then we know it's
likely profitable to lower it, too)
> Anyway, the patch I posted previously would risk re-introducing PR
> 50386 and PR 50326, even though they are very unlikely with just
> bit-fields. So my current working version is the following, but it
> causes failure of libmudflap.c++/pass55-frag.cxx execution test so I'm
> not actually proposing it yet (sigh).
I would not worry about mudflap tests. The patch looks good to my
eyes.
Thanks,
Richard.
> Thanks,
>
> Martin
>
>
> 2012-04-11 Martin Jambor <mjam...@suse.cz>
>
> * tree-sra.c (build_ref_for_model): Create COMPONENT_REFs only for
> bit-fields.
>
> Index: src/gcc/tree-sra.c
> ===================================================================
> *** src.orig/gcc/tree-sra.c
> --- src/gcc/tree-sra.c
> *************** build_ref_for_offset (location_t loc, tr
> *** 1489,1558 ****
> return fold_build2_loc (loc, MEM_REF, exp_type, base, off);
> }
>
> - DEF_VEC_ALLOC_P_STACK (tree);
> - #define VEC_tree_stack_alloc(alloc) VEC_stack_alloc (tree, alloc)
> -
> /* Construct a memory reference to a part of an aggregate BASE at the given
> ! OFFSET and of the type of MODEL. In case this is a chain of references
> ! to component, the function will replicate the chain of COMPONENT_REFs of
> ! the expression of MODEL to access it. GSI and INSERT_AFTER have the same
> ! meaning as in build_ref_for_offset. */
>
> static tree
> build_ref_for_model (location_t loc, tree base, HOST_WIDE_INT offset,
> struct access *model, gimple_stmt_iterator *gsi,
> bool insert_after)
> {
> ! tree type = model->type, t;
> ! VEC(tree,stack) *cr_stack = NULL;
> !
> ! if (TREE_CODE (model->expr) == COMPONENT_REF)
> {
> ! tree expr = model->expr;
> !
> ! /* Create a stack of the COMPONENT_REFs so later we can walk them in
> ! order from inner to outer. */
> ! cr_stack = VEC_alloc (tree, stack, 6);
> !
> ! do {
> ! tree field = TREE_OPERAND (expr, 1);
> ! tree cr_offset = component_ref_field_offset (expr);
> ! HOST_WIDE_INT bit_pos
> ! = tree_low_cst (cr_offset, 1) * BITS_PER_UNIT
> ! + TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field));
>
> ! /* We can be called with a model different from the one associated
> ! with BASE so we need to avoid going up the chain too far. */
> ! if (offset - bit_pos < 0)
> ! break;
> !
> ! offset -= bit_pos;
> ! VEC_safe_push (tree, stack, cr_stack, expr);
> !
> ! expr = TREE_OPERAND (expr, 0);
> ! type = TREE_TYPE (expr);
> ! } while (TREE_CODE (expr) == COMPONENT_REF);
> }
> !
> ! t = build_ref_for_offset (loc, base, offset, type, gsi, insert_after);
> !
> ! if (TREE_CODE (model->expr) == COMPONENT_REF)
> ! {
> ! unsigned i;
> ! tree expr;
> !
> ! /* Now replicate the chain of COMPONENT_REFs from inner to outer. */
> ! FOR_EACH_VEC_ELT_REVERSE (tree, cr_stack, i, expr)
> ! {
> ! tree field = TREE_OPERAND (expr, 1);
> ! t = fold_build3_loc (loc, COMPONENT_REF, TREE_TYPE (field), t, field,
> ! TREE_OPERAND (expr, 2));
> ! }
> !
> ! VEC_free (tree, stack, cr_stack);
> ! }
> !
> ! return t;
> }
>
> /* Construct a memory reference consisting of component_refs and array_refs
> to
> --- 1489,1520 ----
> return fold_build2_loc (loc, MEM_REF, exp_type, base, off);
> }
>
> /* Construct a memory reference to a part of an aggregate BASE at the given
> ! OFFSET and of the same type as MODEL. In case this is a reference to a
> ! bit-field, the function will replicate the last component_ref of model's
> ! expr to access it. GSI and INSERT_AFTER have the same meaning as in
> ! build_ref_for_offset. */
>
> static tree
> build_ref_for_model (location_t loc, tree base, HOST_WIDE_INT offset,
> struct access *model, gimple_stmt_iterator *gsi,
> bool insert_after)
> {
> ! if (TREE_CODE (model->expr) == COMPONENT_REF
> ! && DECL_BIT_FIELD (TREE_OPERAND (model->expr, 1)))
> {
> ! /* This access represents a bit-field. */
> ! tree t, exp_type, fld = TREE_OPERAND (model->expr, 1);
>
> ! offset -= int_bit_position (fld);
> ! exp_type = TREE_TYPE (TREE_OPERAND (model->expr, 0));
> ! t = build_ref_for_offset (loc, base, offset, exp_type, gsi,
> insert_after);
> ! return fold_build3_loc (loc, COMPONENT_REF, TREE_TYPE (fld), t, fld,
> ! NULL_TREE);
> }
> ! else
> ! return build_ref_for_offset (loc, base, offset, model->type,
> ! gsi, insert_after);
> }
>
> /* Construct a memory reference consisting of component_refs and array_refs
> to
>
>
