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++).
>
> The main downside of this change I see is that dumps would be a bit
> more difficult to read and understand when the fields disappear from
> them.
>
> The upsides are:
>
> - the expr field of SRA access was originally intended only for
> debugging (meaning both for compiler-produced debug info and
> debugging SRA). It was never intended to influence the memory
> accesses produced by SRA and when we create them artificially, the
> effects of the particular form are hard to reason about. If we
> ever lower bit-field accesses on gimple level, build_ref_for_model
> could go away completely (yeah, I know I'm getting carried way
> here).
>
> - If something like PR 51528 creeps up again and we need to create
> replacements of type returned by lang_hooks.types.type_for_mode,
> the produced MEM_REFs could simply have this type. OTOH, the
> current COMPONENT_REFs would require to be encapsulated in V_C_Es
> and that is quite a nightmare. I tried it in December, even made
> it work, but it was particularly ugly and needed some quite
> questionable uses of V_C_Es.
>
> - Well, it does the same thing and is much simpler, is it not?
>
> The patch fulfills the criteria to be committed and I can do it soon.
> OTOH, keeping it so on a number of platforms takes quite a lot of time
> (and has uncovered some non-related bugs) so I'd like to know whether
> it's worth it.
>
> Thanks,
>
> Martin
>
>
>
> 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
> {
> ! /* 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?
> ! 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.
Thanks,
Richard.
