* [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)?
@ 2012-04-04 14:06 Martin Jambor
2012-04-04 14:42 ` Richard Guenther
0 siblings, 1 reply; 6+ messages in thread
From: Martin Jambor @ 2012-04-04 14:06 UTC (permalink / raw)
To: GCC Patches; +Cc: Richard Guenther, Eric Botcazou
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 <mjambor@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;
! offset -= int_bit_position (TREE_OPERAND (model->expr, 1));
! 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)?
2012-04-04 14:06 [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)? Martin Jambor
@ 2012-04-04 14:42 ` Richard Guenther
2012-04-12 16:07 ` Martin Jambor
0 siblings, 1 reply; 6+ messages in thread
From: Richard Guenther @ 2012-04-04 14:42 UTC (permalink / raw)
To: Martin Jambor; +Cc: GCC Patches, Eric Botcazou
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 <mjambor@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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)?
2012-04-04 14:42 ` Richard Guenther
@ 2012-04-12 16:07 ` Martin Jambor
2012-04-13 11:53 ` Richard Guenther
0 siblings, 1 reply; 6+ messages in thread
From: Martin Jambor @ 2012-04-12 16:07 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches, Eric Botcazou
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 <mjambor@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.
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).
Thanks,
Martin
2012-04-11 Martin Jambor <mjambor@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)?
2012-04-12 16:07 ` Martin Jambor
@ 2012-04-13 11:53 ` Richard Guenther
2012-04-13 11:57 ` Rainer Orth
0 siblings, 1 reply; 6+ messages in thread
From: Richard Guenther @ 2012-04-13 11:53 UTC (permalink / raw)
To: Martin Jambor; +Cc: GCC Patches, Eric Botcazou
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 <mjambor@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 <mjambor@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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)?
2012-04-13 11:53 ` Richard Guenther
@ 2012-04-13 11:57 ` Rainer Orth
2012-04-13 17:04 ` Martin Jambor
0 siblings, 1 reply; 6+ messages in thread
From: Rainer Orth @ 2012-04-13 11:57 UTC (permalink / raw)
To: Richard Guenther; +Cc: Martin Jambor, GCC Patches, Eric Botcazou
Richard Guenther <rguenther@suse.de> writes:
>> 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.
Are you sure the failure is new? At least for 64-bit at -O,
libmudflap.c++/pass55-frag.cxx already fails right now (cf. PR
libmudflap/49843).
Rainer
--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)?
2012-04-13 11:57 ` Rainer Orth
@ 2012-04-13 17:04 ` Martin Jambor
0 siblings, 0 replies; 6+ messages in thread
From: Martin Jambor @ 2012-04-13 17:04 UTC (permalink / raw)
To: Rainer Orth; +Cc: GCC Patches
On Fri, Apr 13, 2012 at 01:57:33PM +0200, Rainer Orth wrote:
> Richard Guenther <rguenther@suse.de> writes:
>
> >> 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.
>
> Are you sure the failure is new? At least for 64-bit at -O,
> libmudflap.c++/pass55-frag.cxx already fails right now (cf. PR
> libmudflap/49843).
>
Well, my patch makes it fail on all other optimization levels too (the
supposedly no-optimization variant is actually compiled with -O2).
But thanks for pointing me towards the bug, I'll use it as an excuse
to commit the patch nevertheless (mainly because I really cannot see
anything wrong going on there, at least with respect to SRA and this
change).
Thanks,
Martin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-13 17:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-04 14:06 [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)? Martin Jambor
2012-04-04 14:42 ` Richard Guenther
2012-04-12 16:07 ` Martin Jambor
2012-04-13 11:53 ` Richard Guenther
2012-04-13 11:57 ` Rainer Orth
2012-04-13 17:04 ` Martin Jambor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).