Patchwork RFC/A: set_mem_attributes_minus_bitpos tweak

login
register
mail settings
Submitter Richard Sandiford
Date Nov. 3, 2012, 11:53 a.m.
Message ID <87hap6ewcm.fsf@talisman.home>
Download mbox | patch
Permalink /patch/196828/
State New
Headers show

Comments

Richard Sandiford - Nov. 3, 2012, 11:53 a.m.
While working on the insertion and extraction optabs, I came across
some strange-looking behaviour relatd to set_mem_attributes_minus_bitpos.
In gcc.c-torture/execute/20030714-1.c we have:

  struct RenderStyle
  {
    struct NonInheritedFlags 
      {
        union 
          {
            struct 
              {
                unsigned int _display : 4;
                unsigned int _bg_repeat : 2;
                bool _bg_attachment : 1;
                unsigned int _overflow : 4 ;
                unsigned int _vertical_align : 4;
                unsigned int _clear : 2;
                EPosition _position : 2;
                EFloat _floating : 2;
                unsigned int _table_layout : 1;
                bool _flowAroundFloats :1;

                unsigned int _styleType : 3;
                bool _hasHover : 1;
                bool _hasActive : 1;
                bool _clipSpecified : 1;
                unsigned int _unicodeBidi : 2;
                int _unused : 1;
              } f;
            int _niflags;
          };
      } noninherited_flags;
  };
  ...
  RenderStyle g__style;
  ...
    g__style.noninherited_flags.f._position = FIXED;
  ...

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

where "to_rtx" is:

  (mem/c:SI (symbol_ref:DI ("g__style") [flags 0x4] <var_decl 0x7ffff6e4ee40 g__style>) [0 g__style+0 S4 A64])

and "to" is the component_ref.  So far so good.

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.

So the new attributes also start out describing g__style.  Then we have:

  /* 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);
    }

which sets the size to 1 byte (because that's all the bitfield occupies).
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?

Richard


gcc/
	* emit-rtl.c (set_mem_attributes_minus_bitpos): If setting the
	size based on the new tree, always include the bitpos in it.
Eric Botcazou - Nov. 7, 2012, 9:52 a.m.
> 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.

Patch

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	2012-11-01 20:43:37.193362850 +0000
+++ gcc/emit-rtl.c	2012-11-01 20:44:22.248362740 +0000
@@ -1569,7 +1569,7 @@  get_mem_align_offset (rtx mem, unsigned
 set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp,
 				 HOST_WIDE_INT bitpos)
 {
-  HOST_WIDE_INT apply_bitpos = 0;
+  tree expr = NULL_TREE;
   tree type;
   struct mem_attrs attrs, *defattrs, *refattrs;
   addr_space_t as;
@@ -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.  */
@@ -1738,17 +1734,10 @@  set_mem_attributes_minus_bitpos (rtx ref
       /* If this is a decl, set the attributes of the MEM from it.  */
       if (DECL_P (t))
 	{
-	  attrs.expr = t;
+	  expr = t;
 	  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;
 	}
@@ -1770,10 +1759,9 @@  set_mem_attributes_minus_bitpos (rtx ref
       else if (TREE_CODE (t) == COMPONENT_REF
 	       && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
 	{
-	  attrs.expr = t;
+	  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?  */
 	}
@@ -1812,7 +1800,7 @@  set_mem_attributes_minus_bitpos (rtx ref
 
 	  if (DECL_P (t2))
 	    {
-	      attrs.expr = t2;
+	      expr = t2;
 	      attrs.offset_known_p = false;
 	      if (host_integerp (off_tree, 1))
 		{
@@ -1824,18 +1812,16 @@  set_mem_attributes_minus_bitpos (rtx ref
 		  align_computed = true;
 		  attrs.offset_known_p = true;
 		  attrs.offset = ioff;
-		  apply_bitpos = bitpos;
 		}
 	    }
 	  else if (TREE_CODE (t2) == COMPONENT_REF)
 	    {
-	      attrs.expr = t2;
+	      expr = t2;
 	      attrs.offset_known_p = false;
 	      if (host_integerp (off_tree, 1))
 		{
 		  attrs.offset_known_p = true;
 		  attrs.offset = tree_low_cst (off_tree, 1);
-		  apply_bitpos = bitpos;
 		}
 	      /* ??? Any reason the field size would be different than
 		 the size we got from the type?  */
@@ -1846,10 +1832,9 @@  set_mem_attributes_minus_bitpos (rtx ref
       else if (TREE_CODE (t) == MEM_REF 
 	       || TREE_CODE (t) == TARGET_MEM_REF)
 	{
-	  attrs.expr = t;
+	  expr = t;
 	  attrs.offset_known_p = true;
 	  attrs.offset = 0;
-	  apply_bitpos = bitpos;
 	}
 
       if (!align_computed)
@@ -1861,17 +1846,22 @@  set_mem_attributes_minus_bitpos (rtx ref
   else
     as = TYPE_ADDR_SPACE (type);
 
-  /* 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.  */
-  if (apply_bitpos)
+  if (expr)
     {
-      gcc_assert (attrs.offset_known_p);
-      attrs.offset -= apply_bitpos / BITS_PER_UNIT;
-      if (attrs.size_known_p)
-	attrs.size += apply_bitpos / BITS_PER_UNIT;
+      attrs.expr = expr;
+      /* If we modified OFFSET based on T, then subtract the outstanding
+	 bit position offset.  */
+      if (attrs.offset_known_p)
+	attrs.offset -= bitpos / BITS_PER_UNIT;
     }
-
+  if (host_integerp (new_size, 1))
+    {
+      attrs.size_known_p = true;
+      /* Include the outstanding bit offset in the size, so that the
+	 initial reference includes the leading gap and T.  */
+      attrs.size = tree_low_cst (new_size, 1) + bitpos / BITS_PER_UNIT;
+    }
+	
   /* Now set the attributes we computed above.  */
   attrs.addrspace = as;
   set_mem_attrs (ref, &attrs);