diff mbox

Fix PR55882

Message ID alpine.LNX.2.00.1301101142230.6889@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Jan. 10, 2013, 10:52 a.m. UTC 
On Thu, 10 Jan 2013, Richard Biener wrote:

> On Thu, 10 Jan 2013, Richard Biener wrote:
> 
> > On Wed, 9 Jan 2013, Eric Botcazou wrote:
> > 
> > > > This fixes PR55882 - set_mem_attributes_minus_bitpos misses to
> > > > account for the to-be applied bitpos when computing MEM_ALIGN.
> > > > It extracts alignment from 't' instead of &t - bitpos.
> > > > 
> > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, bootstrap
> > > > and regtest running on mips.
> > > > 
> > > > Does this look sensible?
> > > 
> > > I'm not sure, bitpos isn't taken into account in the other cases when the 
> > > alignment is computed.  adjust_address_1 is supposed to properly adjust the 
> > > alignment by the time the offset is applied.
> > 
> > Yes, but alignment adjustment assumes the alignment is that of
> > the actual MEM, not that of the MEM_EXPR.  Thus, MEM_ALIGN is the
> > alignment of MEM_EXPR - MEM_OFFSET.
> > 
> > It's true that the other paths setting align_computed have the
> > same issue.  I can produce a followup (or preparatory patch)
> > that at least removes the code that is redundant.  For example
> > 
> >       /* If this is a decl, set the attributes of the MEM from it.  */
> >       if (DECL_P (t))
> >         {
> >           attrs.expr = t;
> >           attrs.offset_known_p = true;
> >           attrs.offset = 0;
> >           apply_bitpos = bitpos;
> >           new_size = DECL_SIZE_UNIT (t);
> >           attrs.align = DECL_ALIGN (t);
> >           align_computed = true;
> >         }
> > 
> > the alignment computation can be handled by the get_object_alignment
> > path just fine, no need to duplicate it here, likewise for the
> > CONSTANT_CLASS_P case.  The ARRAY_REF path is scary enough to
> > me (I'd rather drop it completely ... it's probably designed to
> > make more cases covered by nonoverlapping_component_refs_p?).
> > At least handling of DECL/COMPONENT_REF below the ARRAY_REFs
> > should be commonized - for example the COMPONENT_REF case
> > 
> >           else if (TREE_CODE (t2) == COMPONENT_REF)
> >             {
> >               attrs.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;
> >                 }
> > 
> > doesn't adjust 't' nor does it compute alignment.  So it
> > clearly cannot be that MEM_ALIGN is supposed to be the
> > alignment of MEM_EXPR.
> > 
> > That is - the question boils down to _what_ should MEM_ALIGN
> > be the alignment of?  In all the rest of the compiler it surely
> > is the alignment of XEXP (mem, 0) - to make it differ from that
> > during a brief period of expand_assignment sounds fragile at least.
> > 
> > I'll try to simplify code paths without changing behavior first.
> 
> Going over this what's strange as well is that we adjust MEM_SIZE
> with bitpos, too.  At least when the MEM has non-BLKmode this
> means that MEMs mode and MEM_SIZE is inconsistent?  Or how do
> a MEMs mode and MEM_SIZE relate?
> 
> I came up with the following patch that shouldn't change
> behavior but which makes it easier to follow what happens
> (it will also make bitpos handling with my alignment patch fix
> consistent).
> 
> The patch moves attrs.align "precompute" into the !TYPE_P path
> and for the &DECL / &CONSTANT align compute use get_object_alignment
> which DTRT for it (it's still buggy because we later will use
> MAX of this alignment and the "correct" alignment - but I didn't
> want to change behavior here).
> 
> The patch removes explicit align computation on the DECL_P
> and CONSTANT_CLASS_P paths because that's handled by
> the get_object_alignment path in the same way.  That also
> allows the ARRAY_REF base paths to be simplified and commonized
> (the ARRAY_REF and then DECL_P path will get better align
> computation this way, get_object_alignment has more elaborate
> off_tree handling).
> 
> As followup I'd remove the MEM_REF/TARGET_MEM_REF alignment
> "precompute" completely - it's buggy (due to the later use of MAX).
> That is, consider the below as explanatory intermediate step,
> any real patch would get rid of that "precompute" immediately
> (and then probably not re-use anything we got from default or
> pre-existing MEM_ATTRs but simply overwrite attrs.align.
> See 2nd patch below.

Which needs the bitpos accounting for fix as otherwise
get_object_alignment returns bit-alignment.

And I believe we unconditionally need to account for bitpos,
not only apply_bitpos (which is for MEM_OFFSET and irrelevant
if MEM_EXPR is not set).

Which makes me arrive at the following fix for PR55882
(Martin has completed bootstrap & regtest on SPARC for the
original but agreed incomplete fix).  It's not backportable
to 4.7 this way (the original fix would have worked, even
when slightly altering it to cover also the align_computed
cases).

So - do we want to go this full way or a more conservative
way also applicable literally to 4.7 (conservative in, leaving
some wrong-align holes open)?

Thanks,
Richard.

2013-01-10  Richard Biener  <rguenther@suse.de>

	PR middle-end/55882
	* emit-rtl.c (set_mem_attributes_minus_bitpos): Remove
	alignment computations and rely on get_object_alignment_1
	for the !TYPE_P case and correctly account for bitpos.
	Commonize DECL/COMPONENT_REF handling in the ARRAY_REF path.
diff mbox

Patch

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	(revision 195079)
+++ gcc/emit-rtl.c	(working copy)
@@ -1641,51 +1641,17 @@  set_mem_attributes_minus_bitpos (rtx ref
   if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type))
     attrs.align = MAX (attrs.align, TYPE_ALIGN (type));
 
-  else if (TREE_CODE (t) == MEM_REF)
-    {
-      tree op0 = TREE_OPERAND (t, 0);
-      if (TREE_CODE (op0) == ADDR_EXPR
-	  && (DECL_P (TREE_OPERAND (op0, 0))
-	      || CONSTANT_CLASS_P (TREE_OPERAND (op0, 0))))
-	{
-	  if (DECL_P (TREE_OPERAND (op0, 0)))
-	    attrs.align = DECL_ALIGN (TREE_OPERAND (op0, 0));
-	  else if (CONSTANT_CLASS_P (TREE_OPERAND (op0, 0)))
-	    {
-	      attrs.align = TYPE_ALIGN (TREE_TYPE (TREE_OPERAND (op0, 0)));
-#ifdef CONSTANT_ALIGNMENT
-	      attrs.align = CONSTANT_ALIGNMENT (TREE_OPERAND (op0, 0),
-						attrs.align);
-#endif
-	    }
-	  if (TREE_INT_CST_LOW (TREE_OPERAND (t, 1)) != 0)
-	    {
-	      unsigned HOST_WIDE_INT ioff
-		= TREE_INT_CST_LOW (TREE_OPERAND (t, 1));
-	      unsigned HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT;
-	      attrs.align = MIN (aoff, attrs.align);
-	    }
-	}
-      else
-	/* ??? This isn't fully correct, we can't set the alignment from the
-	   type in all cases.  */
-	attrs.align = MAX (attrs.align, TYPE_ALIGN (type));
-    }
-
-  else if (TREE_CODE (t) == TARGET_MEM_REF)
-    /* ??? This isn't fully correct, we can't set the alignment from the
-       type in all cases.  */
-    attrs.align = MAX (attrs.align, TYPE_ALIGN (type));
-
   /* If the size is known, we can set that.  */
   tree new_size = TYPE_SIZE_UNIT (type);
 
+  /* The address-space is that of the type.  */
+  as = TYPE_ADDR_SPACE (type);
+
   /* If T is not a type, we may be able to deduce some more information about
      the expression.  */
   if (! TYPE_P (t))
     {
       tree base;
-      bool align_computed = false;
 
       if (TREE_THIS_VOLATILE (t))
 	MEM_VOLATILE_P (ref) = 1;
@@ -1715,6 +1681,7 @@  set_mem_attributes_minus_bitpos (rtx ref
 	      && TREE_STATIC (base))
 	    MEM_READONLY_P (ref) = 1;
 
+	  /* Address-space information is on the base object.  */
 	  if (TREE_CODE (base) == MEM_REF
 	      || TREE_CODE (base) == TARGET_MEM_REF)
 	    as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (base,
@@ -1722,8 +1689,6 @@  set_mem_attributes_minus_bitpos (rtx ref
 	  else
 	    as = TYPE_ADDR_SPACE (TREE_TYPE (base));
 	}
-      else
-	as = TYPE_ADDR_SPACE (type);
 
       /* If this expression uses it's parent's alias set, mark it such
 	 that we won't change it.  */
@@ -1738,19 +1703,11 @@  set_mem_attributes_minus_bitpos (rtx ref
 	  attrs.offset = 0;
 	  apply_bitpos = bitpos;
 	  new_size = DECL_SIZE_UNIT (t);
-	  attrs.align = DECL_ALIGN (t);
-	  align_computed = true;
 	}
 
-      /* If this is a constant, we know the alignment.  */
+      /* ???  If we end up with a constant here do record a MEM_EXPR.  */
       else if (CONSTANT_CLASS_P (t))
-	{
-	  attrs.align = TYPE_ALIGN (type);
-#ifdef CONSTANT_ALIGNMENT
-	  attrs.align = CONSTANT_ALIGNMENT (t, attrs.align);
-#endif
-	  align_computed = true;
-	}
+	;
 
       /* If this is a field reference, record it.  */
       else if (TREE_CODE (t) == COMPONENT_REF)
@@ -1795,24 +1752,8 @@  set_mem_attributes_minus_bitpos (rtx ref
 	    }
 	  while (TREE_CODE (t2) == ARRAY_REF);
 
-	  if (DECL_P (t2))
-	    {
-	      attrs.expr = t2;
-	      attrs.offset_known_p = false;
-	      if (host_integerp (off_tree, 1))
-		{
-		  HOST_WIDE_INT ioff = tree_low_cst (off_tree, 1);
-		  HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT;
-		  attrs.align = DECL_ALIGN (t2);
-		  if (aoff && (unsigned HOST_WIDE_INT) aoff < attrs.align)
-	            attrs.align = aoff;
-		  align_computed = true;
-		  attrs.offset_known_p = true;
-		  attrs.offset = ioff;
-		  apply_bitpos = bitpos;
-		}
-	    }
-	  else if (TREE_CODE (t2) == COMPONENT_REF)
+	  if (DECL_P (t2)
+	      || TREE_CODE (t2) == COMPONENT_REF)
 	    {
 	      attrs.expr = t2;
 	      attrs.offset_known_p = false;
@@ -1822,9 +1763,8 @@  set_mem_attributes_minus_bitpos (rtx ref
 		  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?  */
 	    }
+	  /* Else do not record a MEM_EXPR.  */
 	}
 
       /* If this is an indirect reference, record it.  */
@@ -1837,14 +1777,15 @@  set_mem_attributes_minus_bitpos (rtx ref
 	  apply_bitpos = bitpos;
 	}
 
-      if (!align_computed)
-	{
-	  unsigned int obj_align = get_object_alignment (t);
-	  attrs.align = MAX (attrs.align, obj_align);
-	}
+      /* Compute the alignment.  */
+      unsigned int obj_align;
+      unsigned HOST_WIDE_INT obj_bitpos;
+      get_object_alignment_1 (t, &obj_align, &obj_bitpos);
+      obj_bitpos = (obj_bitpos - bitpos) & (obj_align - 1);
+      if (obj_bitpos != 0)
+	obj_align = (obj_bitpos & -obj_bitpos);
+      attrs.align = obj_align;
     }
-  else
-    as = TYPE_ADDR_SPACE (type);
 
   if (host_integerp (new_size, 1))
     {