Re: [RFC] Should SRA stop producing COMPONENT_REF for non-bit-fields (again)?

[Richard Guenther <rguenther@suse.de>]

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