Re: RFC/A: set_mem_attributes_minus_bitpos tweak

[Eric Botcazou <ebotcazou at adacore dot com>]

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

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

-- 
Eric Botcazou