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. 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 <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