Re: RFC/A: set_mem_attributes_minus_bitpos tweak

[Richard Sandiford <rdsandiford at googlemail dot com>]

Eric Botcazou <ebotcazou@adacore.com> writes:
>> expand_assignment calls:
>> 
>>            if (MEM_P (to_rtx))
>>              {
>>                /* If the field is at offset zero, we could have been given
>> the DECL_RTX of the parent struct.  Don't munge it.  */ to_rtx =
>> shallow_copy_rtx (to_rtx);
>> 
>>                set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
>>                ...
>
> The MEM_KEEP_ALIAS_SET_P line seems to be redundant here (but not the 
> MEM_VOLATILE_P line).

Good catch.  I've removed that in the patch below.

>> But set_mem_attributes_minus_bitpos has:
>> 
>>   /* Default values from pre-existing memory attributes if present.  */
>>   refattrs = MEM_ATTRS (ref);
>>   if (refattrs)
>>     {
>>       /* ??? Can this ever happen?  Calling this routine on a MEM that
>> 	 already carries memory attributes should probably be invalid.  */
>>       attrs.expr = refattrs->expr;
>>       attrs.offset_known_p = refattrs->offset_known_p;
>>       attrs.offset = refattrs->offset;
>>       attrs.size_known_p = refattrs->size_known_p;
>>       attrs.size = refattrs->size;
>>       attrs.align = refattrs->align;
>>     }
>> 
>> which of course applies in this case: we have a MEM for g__style.
>> I would expect many calls from this site are for MEMs with an
>> existing MEM_EXPR.
>
> Indeed.  Not very clear what to do though.
>
>> Then later:
>> 
>>       /* If this is a field reference and not a bit-field, record it.  */
>>       /* ??? There is some information that can be gleaned from bit-fields,
>> 	 such as the word offset in the structure that might be modified.
>> 	 But skip it for now.  */
>>       else if (TREE_CODE (t) == COMPONENT_REF
>> 	       && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
>> 
>> so we leave the offset and expr alone.  The end result is:
>> 
>>   (mem/j/c:SI (symbol_ref:DI ("g__style") [flags 0x4] <var_decl
>> 0x7ffff6e4ee40 g__style>) [0 g__style+0 S1 A64])
>> 
>> an SImode reference to the first byte (and only the first byte) of g__style.
>> Then, when we apply adjust_bitfield_address, it looks like we're moving
>> past the end of the region and so we drop the MEM_EXPR.
>> 
>> In cases where set_mem_attributes_minus_bitpos does set MEM_EXPR based
>> on the new tree, it also adds the bitpos to the size.  But I think we
>> should do that whenever we set the size based on the new tree,
>> regardless of whether we were able to record a MEM_EXPR too.
>> 
>> That said, this code has lots of ???s in it, so I'm not exactly
>> confident about this change.  Thoughts?
>
> It also seems a little bold to me.  Since we now have the new processing of 
> MEM_EXPR for bitfields, can't we remove the ! DECL_BIT_FIELD check?
>
>       /* If this is a field reference and not a bit-field, record it.  */
>       /* ??? There is some information that can be gleaned from bit-fields,
> 	 such as the word offset in the structure that might be modified.
> 	 But skip it for now.  */
>       else if (TREE_CODE (t) == COMPONENT_REF
> 	       && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
> 	{
> 	  attrs.expr = t;
> 	  attrs.offset_known_p = true;
> 	  attrs.offset = 0;
> 	  apply_bitpos = bitpos;
> 	  /* ??? Any reason the field size would be different than
> 	     the size we got from the type?  */
> 	}
>
> This would mean removing the first ??? comment.  As for the second ??? 
> comment, the answer is easy: because that's pretty much what defines a 
> bitfield!  The size is DECL_SIZE_UNIT and not TYPE_SIZE_UNIT for them.

I don't know this area well, so I'll have to take your word for it that
handling DECL_BIT_FIELDs is now safe.  I certainly won't argue against
removing two ??? comments though :-)

Tested on x86_64-linux-gnu.  OK to install?

Richard


gcc/
	* expr.c (expand_assignment): Don't set MEM_KEEP_ALIAS_SET_P here.
	* emit-rtl.c (set_mem_attributes_minus_bitpos): Handle DECL_BIT_FIELDs,
	using their size instead of the COMPONENT_REF's.

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	2012-11-10 22:17:24.797602770 +0000
+++ gcc/emit-rtl.c	2012-11-10 22:19:17.779228832 +0000
@@ -1679,11 +1679,7 @@ set_mem_attributes_minus_bitpos (rtx ref
     attrs.align = MAX (attrs.align, TYPE_ALIGN (type));
 
   /* If the size is known, we can set that.  */
-  if (TYPE_SIZE_UNIT (type) && host_integerp (TYPE_SIZE_UNIT (type), 1))
-    {
-      attrs.size_known_p = true;
-      attrs.size = tree_low_cst (TYPE_SIZE_UNIT (type), 1);
-    }
+  tree new_size = TYPE_SIZE_UNIT (type);
 
   /* If T is not a type, we may be able to deduce some more information about
      the expression.  */
@@ -1742,13 +1738,7 @@ set_mem_attributes_minus_bitpos (rtx ref
 	  attrs.offset_known_p = true;
 	  attrs.offset = 0;
 	  apply_bitpos = bitpos;
-	  if (DECL_SIZE_UNIT (t) && host_integerp (DECL_SIZE_UNIT (t), 1))
-	    {
-	      attrs.size_known_p = true;
-	      attrs.size = tree_low_cst (DECL_SIZE_UNIT (t), 1);
-	    }
-	  else
-	    attrs.size_known_p = false;
+	  new_size = DECL_SIZE_UNIT (t);
 	  attrs.align = DECL_ALIGN (t);
 	  align_computed = true;
 	}
@@ -1763,19 +1753,15 @@ set_mem_attributes_minus_bitpos (rtx ref
 	  align_computed = true;
 	}
 
-      /* If this is a field reference and not a bit-field, record it.  */
-      /* ??? There is some information that can be gleaned from bit-fields,
-	 such as the word offset in the structure that might be modified.
-	 But skip it for now.  */
-      else if (TREE_CODE (t) == COMPONENT_REF
-	       && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
+      /* If this is a field reference, record it.  */
+      else if (TREE_CODE (t) == COMPONENT_REF)
 	{
 	  attrs.expr = t;
 	  attrs.offset_known_p = true;
 	  attrs.offset = 0;
 	  apply_bitpos = bitpos;
-	  /* ??? Any reason the field size would be different than
-	     the size we got from the type?  */
+	  if (DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
+	    new_size = DECL_SIZE_UNIT (TREE_OPERAND (t, 1));
 	}
 
       /* If this is an array reference, look for an outer field reference.  */
@@ -1861,6 +1847,12 @@ set_mem_attributes_minus_bitpos (rtx ref
   else
     as = TYPE_ADDR_SPACE (type);
 
+  if (host_integerp (new_size, 1))
+    {
+      attrs.size_known_p = true;
+      attrs.size = tree_low_cst (new_size, 1);
+    }
+
   /* If we modified OFFSET based on T, then subtract the outstanding
      bit position offset.  Similarly, increase the size of the accessed
      object to contain the negative offset.  */
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	2012-11-10 22:19:14.000000000 +0000
+++ gcc/expr.c	2012-11-10 22:21:17.615919793 +0000
@@ -4821,8 +4821,6 @@ expand_assignment (tree to, tree from, b
 		 done for MEM.  Also set MEM_KEEP_ALIAS_SET_P if needed.  */
 	      if (volatilep)
 		MEM_VOLATILE_P (to_rtx) = 1;
-	      if (component_uses_parent_alias_set (to))
-		MEM_KEEP_ALIAS_SET_P (to_rtx) = 1;
 	    }
 
 	  if (optimize_bitfield_assignment_op (bitsize, bitpos,