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

[Martin Jambor]

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