[C++0x] contiguous bitfields race implementation

> Some comments.
>
>        /* If we have a bit-field with a bitsize>  0... */
>        if (DECL_BIT_FIELD_TYPE (fld)
> 	&&  tree_low_cst (DECL_SIZE (fld), 1)>  0)
>
> I think we can check bitsize != 0, thus
>
> &&  !integer_zerop (DECL_SIZE (fld))

Done

> 	  /* Short-circuit out if we have the max bits allowed.  */
> 	  if (cumulative_bitsize>= MAX_FIXED_MODE_SIZE)
> 	    {
> 	      *maxbits = MAX_FIXED_MODE_SIZE;
> 	      /* Calculate byte offset to the beginning of the bit region.  */
> 	      gcc_assert (start_bitpos % BITS_PER_UNIT == 0);
> 	      *offset = fold_build2 (PLUS_EXPR, TREE_TYPE (start_offset),
> 				     start_offset,
> 				     build_int_cst (integer_type_node,
> 						    start_bitpos / BITS_PER_UNIT));
> 	      return;
>
> apart from the *offset calculation being redundant, *offset + maxbits
> may not include the referenced field.  How do you plan to find
> an "optimal" window for such access? (*)

Actually offset is always needed because we use it in store_bit_field() 
to adjust the memory reference up to the bit region.  However... I have 
removed the MAX_FIXED_MODE_SIZE constrain all throughout.  See 
explanation below.

>        if (!DECL_BIT_FIELD_TYPE (fld)
>           || integer_zerop (DECL_SIZE (fld)))
>          break;
>
> we know that we'll eventually find field.

Good catch.  Done.

>    /* If we found the end of the bit field sequence, include the
>       padding up to the next field...  */
>    if (fld)
>      {
>
> could be a non-FIELD_DECL, you have to skip those first.

I can't find an example of a non-FIELD_DECL here for the life of me. 
I've run numerous tests and can't trigger one.  Do you have one in mind 
so I can handle it?

> Note that another complication comes to my mind now - the offset
> field of a COMPONENT_REF is used to specify a variable offset
> and has to be used, if present, instead of DECL_FIELD_OFFSET.
> Thus your building of COMPONENT_REFs to then pass them to
> get_inner_reference is broken.  As you are in generic code and not
> in the C++ frontend I believe you have to properly handle this case
> (may I suggest to, at the start of the function, simply return a
> minimum byte-aligned blob for the case that there is a variable
> offset to the bitfield?)

Done.

BTW, where would this happen?  I'd like an actual test to stick in 
there.  Is this for non C/C++?

>    /* ...otherwise, this is the last element in the structure.  */
>    else
>      {
>        /* Include the padding at the end of structure.  */
>        *maxbits = TREE_INT_CST_LOW (TYPE_SIZE (record_type))
> 	- TREE_INT_CST_LOW (start_bitpos_direct_parent);
>        if (*maxbits>  MAX_FIXED_MODE_SIZE)
> 	*maxbits = MAX_FIXED_MODE_SIZE;
>      }
>
> with Andrews answer this is invalid.  You can (and should) at most do
>
>    else
>      *maxbits = (*maxbits + BITS_PER_UNIT - 1)&  ~(BITS_PER_UNIT - 1);

I have done this, but I have also removed the constraint to 
MAX_FIXED_MODE_SIZE.  See below.

> There is still the general issue of packed bitfields which will probably
> make the issue of the computed group not covering all of field more
> prominent (esp. if you limit to MAX_FIXED_MODE_SIZE - consider
> struct __attribute__((packed)) { long long : 1; long long a : 64; char
> c; } where
> a does not fit in a DImode mem but crosses it.  Why constrain
> *maxbits to MAX_FIXED_MODE_SIZE at all?  Shoudln't the *offset,
> *maxbits pair just constrain what the caller does, not force it to actually
> use an access covering that full range (does it?)?

Well, I have removed the MAX_FIXED_MODE_SIZE restriction, since every 
offset adjustment in the bit field machinery will need a corresponding 
MAXBITS adjustment.  We can end up reducing MAXBITS into negative 
territory.  So we really have to keep track of the actual number of 
bits.  So, we've converged, just for different reasons :).

I have bootstrapped the compiler with the bitfield restrictions in 
place.  This unearthed a few corner cases, which we are now handling 
correctly with this revision.  So, please take a look at the entire 
patch, as there are small changes throughout.

I am including both the entire patch, and the get_bit_range() function 
separately, to make it easier to review.

Thanks.
/* In the C++ memory model, consecutive non-zero bit fields in a
   structure are considered one memory location.

   Given a COMPONENT_REF, this function calculates the byte offset
   from the containing object to the start of the contiguous bit
   region containing the field in question.  This byte offset is
   returned in *OFFSET.

   The maximum number of bits that can be addressed while storing into
   the COMPONENT_REF is returned in *MAXBITS.  This number is the
   number of bits in the contiguous bit region, including any
   padding.  */

static void
get_bit_range (tree exp, tree *offset, HOST_WIDE_INT *maxbits)
{
  tree field, record_type, fld;
  bool prev_field_is_bitfield;
  tree start_offset;
  tree start_bitpos_direct_parent = NULL_TREE;
  HOST_WIDE_INT start_bitpos;
  HOST_WIDE_INT cumulative_bitsize = 0;
  /* First field of the bitfield group containing the bitfield we are
     referencing.  */
  tree bitregion_start;

  HOST_WIDE_INT tbitsize;
  enum machine_mode tmode;
  int tunsignedp, tvolatilep;

  gcc_assert (TREE_CODE (exp) == COMPONENT_REF);

  /* Be as conservative as possible on variable offsets.  */
  if (TREE_OPERAND (exp, 2)
      && !host_integerp (TREE_OPERAND (exp, 2), 1))
    {
      *offset = TREE_OPERAND (exp, 2);
      *maxbits = BITS_PER_UNIT;
      return;
    }

  /* Bit field we're storing into.  */
  field = TREE_OPERAND (exp, 1);
  record_type = DECL_FIELD_CONTEXT (field);

  /* Find the bitfield group containing the field in question, and set
     BITREGION_START to the start of the group.  */
  prev_field_is_bitfield = false;
  bitregion_start = NULL_TREE;
  for (fld = TYPE_FIELDS (record_type); fld; fld = DECL_CHAIN (fld))
    {
      if (TREE_CODE (fld) != FIELD_DECL)
	continue;

      /* If we have a non-zero bit-field.  */
      if (DECL_BIT_FIELD_TYPE (fld)
	  && !integer_zerop (DECL_SIZE (fld)))
	{
	  if (!prev_field_is_bitfield)
	    {
	      bitregion_start = fld;
	      prev_field_is_bitfield = true;
	    }
	}
      else
	prev_field_is_bitfield = false;
      if (fld == field)
	break;
    }
  gcc_assert (bitregion_start);
  gcc_assert (fld);

  /* Save the starting position of the bitregion.  */
  get_inner_reference (build3 (COMPONENT_REF,
			       TREE_TYPE (exp),
			       TREE_OPERAND (exp, 0),
			       bitregion_start, NULL_TREE),
		       &tbitsize, &start_bitpos, &start_offset,
		       &tmode, &tunsignedp, &tvolatilep, true);

  if (!start_offset)
    start_offset = size_zero_node;
  /* Calculate byte offset to the beginning of the bit region.  */
  /* OFFSET = START_OFFSET + (START_BITPOS / BITS_PER_UNIT) */
  gcc_assert (start_bitpos % BITS_PER_UNIT == 0);
  *offset = fold_build2 (PLUS_EXPR, TREE_TYPE (start_offset),
			 start_offset,
			 build_int_cst (integer_type_node,
					start_bitpos / BITS_PER_UNIT));

  /* Save the bit offset of the current structure.  */
  start_bitpos_direct_parent = DECL_FIELD_BIT_OFFSET (bitregion_start);

  /* Count the bitsize of the bitregion containing the field in question.  */
  cumulative_bitsize = 0;
  for (fld = bitregion_start; fld; fld = DECL_CHAIN (fld))
    {
      if (TREE_CODE (fld) != FIELD_DECL)
	continue;

      if (!DECL_BIT_FIELD_TYPE (fld)
	  || integer_zerop (DECL_SIZE (fld)))
	break;

      cumulative_bitsize += tree_low_cst (DECL_SIZE (fld), 1);
    }

  /* If we found the end of the bit field sequence, include the
     padding up to the next field...  */
  if (fld)
    {
      tree end_offset, maxbits_tree;
      HOST_WIDE_INT end_bitpos;

      /* Calculate bitpos and offset of the next field.  */
      get_inner_reference (build3 (COMPONENT_REF,
				   TREE_TYPE (exp),
				   TREE_OPERAND (exp, 0),
				   fld, NULL_TREE),
			   &tbitsize, &end_bitpos, &end_offset,
			   &tmode, &tunsignedp, &tvolatilep, true);
      gcc_assert (end_bitpos % BITS_PER_UNIT == 0);

      if (end_offset)
	{
	  tree type = TREE_TYPE (end_offset), end;

	  /* Calculate byte offset to the end of the bit region.  */
	  end = fold_build2 (PLUS_EXPR, type,
			     end_offset,
			     build_int_cst (type,
					    end_bitpos / BITS_PER_UNIT));
	  maxbits_tree = fold_build2 (MINUS_EXPR, type, end, *offset);
	}
      else
	maxbits_tree = build_int_cst (integer_type_node,
				      end_bitpos - start_bitpos);

      *maxbits = TREE_INT_CST_LOW (maxbits_tree);
    }
  /* ...otherwise, this is the last element in the structure.  */
  else
    {
      /* Include the padding at the end of structure.  */
      *maxbits = TREE_INT_CST_LOW (TYPE_SIZE (record_type))
	- TREE_INT_CST_LOW (start_bitpos_direct_parent);
      /* Round up to the next byte.  */
      *maxbits = (*maxbits + BITS_PER_UNIT - 1) & ~(BITS_PER_UNIT - 1);
    }
}
* machmode.h (get_best_mode): Remove 2 arguments.
	* fold-const.c (optimize_bit_field_compare): Same.
	(fold_truthop): Same.
	* expr.c (store_field): Change argument types in prototype.
	(emit_group_store): Change argument types to store_bit_field call.
	(copy_blkmode_from_reg): Same.
	(write_complex_part): Same.
	(optimize_bitfield_assignment_op): Change argument types.
	Change arguments to get_best_mode.
	(get_bit_range): Rewrite.
	(expand_assignment): Adjust new call to get_bit_range.
	Adjust bitregion_offset when to_rtx is changed.
	Adjust calls to store_field with new argument types.
	(store_field): New argument types.
	Adjust calls to store_bit_field with new arguments.
	* expr.h (store_bit_field): Change argument types.
	* stor-layout.c (get_best_mode): Remove use of bitregion* arguments.
	* expmed.c (store_bit_field_1): Change argument types.
	Do not calculate maxbits.
	Adjust bitregion_maxbits if offset changes.
	(store_bit_field): Change argument types.
	Adjust address taking into account bitregion_offset.
	(store_fixed_bit_field): Change argument types.
	Do not calculate maxbits.
	(store_split_bit_field): Change argument types.
	(extract_bit_field_1): Adjust arguments to get_best_mode.
	(extract_fixed_bit_field): Same.

[C++0x] contiguous bitfields race implementation

Commit Message

Patch