[PATCH] Fix PR55882

[PATCH] Fix PR55882

[Richard Biener]

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?

Thanks,
Richard.

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

	PR middle-end/55882
	* emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
	account for the to-be applied bitpos when computing alignment.

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	(revision 195014)
+++ gcc/emit-rtl.c	(working copy)
@@ -1839,7 +1839,12 @@ set_mem_attributes_minus_bitpos (rtx ref
 
       if (!align_computed)
 	{
-	  unsigned int obj_align = get_object_alignment (t);
+	  unsigned int obj_align;
+	  unsigned HOST_WIDE_INT obj_bitpos;
+	  get_object_alignment_1 (t, &obj_align, &obj_bitpos);
+	  obj_bitpos = (obj_bitpos - apply_bitpos) & (obj_align - 1);
+	  if (obj_bitpos != 0)
+	    obj_align = (obj_bitpos & -obj_bitpos);
 	  attrs.align = MAX (attrs.align, obj_align);
 	}
     }

Re: [PATCH] Fix PR55882

[Eric Botcazou]

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

-- 
Eric Botcazou

Re: [PATCH] Fix PR55882

[Richard Biener]

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.

Thanks,
Richard.

Re: [PATCH] Fix PR55882

[Richard Biener]

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.

Richard.

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

	* emit-rtl.c (set_mem_attributes_minus_bitpos): Move MEM_REF
	handling completely into the !TYPE_P handling part and use
	get_object_alignment for its alignment computation.  Always
	rely on get_object_alignment for final alignment computation.

Index: gcc/emit-rtl.c
===================================================================
*** gcc/emit-rtl.c	(revision 195079)
--- gcc/emit-rtl.c	(working copy)
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1641,1691 ****
    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);
  
    /* 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;
--- 1641,1657 ----
    if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type))
      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;
  
        if (TREE_THIS_VOLATILE (t))
  	MEM_VOLATILE_P (ref) = 1;
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1715,1720 ****
--- 1681,1687 ----
  	      && 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,
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1722,1729 ****
  	  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.  */
--- 1689,1694 ----
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1738,1756 ****
  	  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.  */
        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)
--- 1703,1713 ----
  	  attrs.offset = 0;
  	  apply_bitpos = bitpos;
  	  new_size = DECL_SIZE_UNIT (t);
  	}
  
!       /* ???  If we end up with a constant here do record a MEM_EXPR.  */
        else if (CONSTANT_CLASS_P (t))
! 	;
  
        /* If this is a field reference, record it.  */
        else if (TREE_CODE (t) == COMPONENT_REF)
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1795,1818 ****
  	    }
  	  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)
  	    {
  	      attrs.expr = t2;
  	      attrs.offset_known_p = false;
--- 1752,1759 ----
  	    }
  	  while (TREE_CODE (t2) == ARRAY_REF);
  
! 	  if (DECL_P (t2)
! 	      || TREE_CODE (t2) == COMPONENT_REF)
  	    {
  	      attrs.expr = t2;
  	      attrs.offset_known_p = false;
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1822,1850 ****
  		  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?  */
  	    }
  	}
  
        /* If this is an indirect reference, record it.  */
        else if (TREE_CODE (t) == MEM_REF 
  	       || TREE_CODE (t) == TARGET_MEM_REF)
  	{
  	  attrs.expr = t;
  	  attrs.offset_known_p = true;
  	  attrs.offset = 0;
  	  apply_bitpos = bitpos;
  	}
  
!       if (!align_computed)
! 	{
! 	  unsigned int obj_align = get_object_alignment (t);
! 	  attrs.align = MAX (attrs.align, obj_align);
! 	}
      }
-   else
-     as = TYPE_ADDR_SPACE (type);
  
    if (host_integerp (new_size, 1))
      {
--- 1763,1813 ----
  		  attrs.offset = tree_low_cst (off_tree, 1);
  		  apply_bitpos = bitpos;
  		}
  	    }
+ 	  /* Else do not record a MEM_EXPR.  */
  	}
  
        /* If this is an indirect reference, record it.  */
        else if (TREE_CODE (t) == MEM_REF 
  	       || TREE_CODE (t) == TARGET_MEM_REF)
  	{
+ 	  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))))
+ 		{
+ 		  attrs.align = get_object_alignment (TREE_OPERAND (op0, 0));
+ 		  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));
+ 
  	  attrs.expr = t;
  	  attrs.offset_known_p = true;
  	  attrs.offset = 0;
  	  apply_bitpos = bitpos;
  	}
  
!       unsigned int obj_align = get_object_alignment (t);
!       attrs.align = MAX (attrs.align, obj_align);
      }
  
    if (host_integerp (new_size, 1))
      {



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

	* emit-rtl.c (set_mem_attributes_minus_bitpos): Remove
	alignment computations and rely on get_object_alignment
	for the !TYPE_P case.  Commonize DECL/COMPONENT_REF handling
	in the ARRAY_REF path.

Index: gcc/emit-rtl.c
===================================================================
*** gcc/emit-rtl.c	(revision 195079)
--- gcc/emit-rtl.c	(working copy)
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1641,1691 ****
    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);
  
    /* 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;
--- 1641,1657 ----
    if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type))
      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;
  
        if (TREE_THIS_VOLATILE (t))
  	MEM_VOLATILE_P (ref) = 1;
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1715,1720 ****
--- 1681,1687 ----
  	      && 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,
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1722,1729 ****
  	  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.  */
--- 1689,1694 ----
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1738,1756 ****
  	  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.  */
        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)
--- 1703,1713 ----
  	  attrs.offset = 0;
  	  apply_bitpos = bitpos;
  	  new_size = DECL_SIZE_UNIT (t);
  	}
  
!       /* ???  If we end up with a constant here do record a MEM_EXPR.  */
        else if (CONSTANT_CLASS_P (t))
! 	;
  
        /* If this is a field reference, record it.  */
        else if (TREE_CODE (t) == COMPONENT_REF)
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1795,1818 ****
  	    }
  	  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)
  	    {
  	      attrs.expr = t2;
  	      attrs.offset_known_p = false;
--- 1752,1759 ----
  	    }
  	  while (TREE_CODE (t2) == ARRAY_REF);
  
! 	  if (DECL_P (t2)
! 	      || TREE_CODE (t2) == COMPONENT_REF)
  	    {
  	      attrs.expr = t2;
  	      attrs.offset_known_p = false;
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1822,1830 ****
  		  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?  */
  	    }
  	}
  
        /* If this is an indirect reference, record it.  */
--- 1763,1770 ----
  		  attrs.offset = tree_low_cst (off_tree, 1);
  		  apply_bitpos = bitpos;
  		}
  	    }
+ 	  /* Else do not record a MEM_EXPR.  */
  	}
  
        /* If this is an indirect reference, record it.  */
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1837,1850 ****
  	  apply_bitpos = bitpos;
  	}
  
!       if (!align_computed)
! 	{
! 	  unsigned int obj_align = get_object_alignment (t);
! 	  attrs.align = MAX (attrs.align, obj_align);
! 	}
      }
-   else
-     as = TYPE_ADDR_SPACE (type);
  
    if (host_integerp (new_size, 1))
      {
--- 1777,1784 ----
  	  apply_bitpos = bitpos;
  	}
  
!       attrs.align = get_object_alignment (t);
      }
  
    if (host_integerp (new_size, 1))
      {

Re: [PATCH] Fix PR55882

[Richard Biener]

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.

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

Re: [PATCH] Fix PR55882

[Eric Botcazou]

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

Yes, the MEM attributes are incomplete/inconsistent between the call to 
set_mem_attributes_minus_bitpos and the subsequent adjustment by bitpos.
That's why only the final result should matter and need be correct.

Let me try to forge my own opinion on the PR before answering further.

-- 
Eric Botcazou

Re: [PATCH] Fix PR55882

[Michael Matz]

Hi,

On Thu, 10 Jan 2013, Eric Botcazou wrote:

> > 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?
> 
> Yes, the MEM attributes are incomplete/inconsistent between the call to 
> set_mem_attributes_minus_bitpos and the subsequent adjustment by bitpos.

The problem with this intermediate inconsistency is that ...

> That's why only the final result should matter and need be correct.

... this can't be ensured anymore.  In some cases the inconsistency 
between the mem attributes and what XEXP(MEM, 0) represents can't be 
repaired by the later bitpos adjustments, or better it can be only be
repaired by falling back to the conservative side for e.g. the alignment, 
because we don't store enough information in the mem attributes to recover 
what was lost.

When briefly discussing this yesterday I suggested (without having the 
code in front of me) that it be best to simply set the mem attributes only 
at the very end, after having computed to final MEM address including all 
adjustments, instead of generating wrong mem attributes initially and 
hoping for the adjustments to make it come out right at the end.


Ciao,
Michael.

Re: [PATCH] Fix PR55882

[Richard Biener]

On Thu, 10 Jan 2013, Michael Matz wrote:

> Hi,
> 
> On Thu, 10 Jan 2013, Eric Botcazou wrote:
> 
> > > 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?
> > 
> > Yes, the MEM attributes are incomplete/inconsistent between the call to 
> > set_mem_attributes_minus_bitpos and the subsequent adjustment by bitpos.
> 
> The problem with this intermediate inconsistency is that ...
> 
> > That's why only the final result should matter and need be correct.
> 
> ... this can't be ensured anymore.  In some cases the inconsistency 
> between the mem attributes and what XEXP(MEM, 0) represents can't be 
> repaired by the later bitpos adjustments, or better it can be only be
> repaired by falling back to the conservative side for e.g. the alignment, 
> because we don't store enough information in the mem attributes to recover 
> what was lost.
> 
> When briefly discussing this yesterday I suggested (without having the 
> code in front of me) that it be best to simply set the mem attributes only 
> at the very end, after having computed to final MEM address including all 
> adjustments, instead of generating wrong mem attributes initially and 
> hoping for the adjustments to make it come out right at the end.

Btw, if the expand_assignment code is really special we should move
minus-bitpos handling to its sole caller like with the following
(_minus_bitpos variant to be unified with set_mem_attributes and
set_mem_attributes_1 to be introduced that just computes new mem
attributes so that expand_assignment can modify them in a more
cheap way and call set_mem_attrs itself).

Richard.

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 195083)
+++ gcc/expr.c	(working copy)
@@ -4856,7 +4856,16 @@ expand_assignment (tree to, tree from, b
 		 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);
+	      set_mem_attributes_minus_bitpos (to_rtx, to, 0, 0);
+	      if (bitpos / BITS_PER_UNIT != 0)
+		{
+		  if (MEM_OFFSET_KNOWN_P (to_rtx))
+		    set_mem_offset (to_rtx,
+				    MEM_OFFSET (to_rtx) - bitpos / BITS_PER_UNIT);
+		  if (MEM_SIZE_KNOWN_P (to_rtx))
+		    set_mem_size (to_rtx,
+				  MEM_SIZE (to_rtx) + bitpos / BITS_PER_UNIT);
+		}
 
 	      /* Deal with volatile and readonly fields.  The former is only
 		 done for MEM.  Also set MEM_KEEP_ALIAS_SET_P if needed.  */
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	(revision 195083)
+++ gcc/emit-rtl.c	(working copy)
@@ -1566,9 +1566,8 @@ get_mem_align_offset (rtx mem, unsigned
 
 void
 set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp,
-				 HOST_WIDE_INT bitpos)
+				 HOST_WIDE_INT)
 {
-  HOST_WIDE_INT apply_bitpos = 0;
   tree type;
   struct mem_attrs attrs, *defattrs, *refattrs;
   addr_space_t as;
@@ -1736,7 +1735,6 @@ set_mem_attributes_minus_bitpos (rtx ref
 	  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;
@@ -1758,7 +1756,6 @@ set_mem_attributes_minus_bitpos (rtx ref
 	  attrs.expr = t;
 	  attrs.offset_known_p = true;
 	  attrs.offset = 0;
-	  apply_bitpos = bitpos;
 	  if (DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
 	    new_size = DECL_SIZE_UNIT (TREE_OPERAND (t, 1));
 	}
@@ -1809,7 +1806,6 @@ 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)
@@ -1820,7 +1816,6 @@ set_mem_attributes_minus_bitpos (rtx ref
 		{
 		  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?  */
@@ -1834,7 +1829,6 @@ set_mem_attributes_minus_bitpos (rtx ref
 	  attrs.expr = t;
 	  attrs.offset_known_p = true;
 	  attrs.offset = 0;
-	  apply_bitpos = bitpos;
 	}
 
       if (!align_computed)
@@ -1852,17 +1846,6 @@ set_mem_attributes_minus_bitpos (rtx ref
       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.  */
-  if (apply_bitpos)
-    {
-      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;
-    }
-
   /* Now set the attributes we computed above.  */
   attrs.addrspace = as;
   set_mem_attrs (ref, &attrs);

Re: [PATCH] Fix PR55882

[Eric Botcazou]

> The problem with this intermediate inconsistency is that ...
> 
> > That's why only the final result should matter and need be correct.
> 
> ... this can't be ensured anymore.  In some cases the inconsistency
> between the mem attributes and what XEXP(MEM, 0) represents can't be
> repaired by the later bitpos adjustments, or better it can be only be
> repaired by falling back to the conservative side for e.g. the alignment,
> because we don't store enough information in the mem attributes to recover
> what was lost.

Indeed, looking at the history of the code shows that the alignment handling 
via MEM_REF and get_object_alignment is an afterthought.  Originally, it was 
set only in a few specific cases:

  /* We can set the alignment from the type if we are making an object,
     this is an INDIRECT_REF, or if TYPE_ALIGN_OK.  */

and also for DECL_P and CONSTANT_CLASS_P.  That was it, so this was actually 
the alignment of the base and disregarding BITPOS was fine.

> When briefly discussing this yesterday I suggested (without having the
> code in front of me) that it be best to simply set the mem attributes only
> at the very end, after having computed to final MEM address including all
> adjustments, instead of generating wrong mem attributes initially and
> hoping for the adjustments to make it come out right at the end.

Maybe, but for 4.7 (and probably 4.8) I think we should be conservative and 
only fix the problems recently introduced.

So, in the end, I think that we should go either for Richard's initial patch 
in this thread, possibly with a ??? comment along the lines of:

	/* ??? If we haven't computed the alignment yet, compute it now.  Note
	    that, if already computed above, the alignment is that of the base
	    object of T so it is OK to have disregarded BITPOS above.  However,
	    here we are using get_object_alignment_1 which returns the precise
	    alignment of T so we need to account for the BITPOS adjustment.  */
	if (!align_computed)

or for Richard's initial patch + the removal of the MEM_REF block (which is 
midway between Richard's initial patch and Richard's last patch).

-- 
Eric Botcazou

Re: [PATCH] Fix PR55882

[Richard Biener]

On Thu, 10 Jan 2013, Eric Botcazou wrote:

> > The problem with this intermediate inconsistency is that ...
> > 
> > > That's why only the final result should matter and need be correct.
> > 
> > ... this can't be ensured anymore.  In some cases the inconsistency
> > between the mem attributes and what XEXP(MEM, 0) represents can't be
> > repaired by the later bitpos adjustments, or better it can be only be
> > repaired by falling back to the conservative side for e.g. the alignment,
> > because we don't store enough information in the mem attributes to recover
> > what was lost.
> 
> Indeed, looking at the history of the code shows that the alignment handling 
> via MEM_REF and get_object_alignment is an afterthought.  Originally, it was 
> set only in a few specific cases:
> 
>   /* We can set the alignment from the type if we are making an object,
>      this is an INDIRECT_REF, or if TYPE_ALIGN_OK.  */
> 
> and also for DECL_P and CONSTANT_CLASS_P.  That was it, so this was actually 
> the alignment of the base and disregarding BITPOS was fine.

Yes, in these cases bitpos is zero.  It's wrong in the !align_computed
path and possibly in the ARRAY_REF -> DECL_P path (only not as
optimistic as it could be).

> > When briefly discussing this yesterday I suggested (without having the
> > code in front of me) that it be best to simply set the mem attributes only
> > at the very end, after having computed to final MEM address including all
> > adjustments, instead of generating wrong mem attributes initially and
> > hoping for the adjustments to make it come out right at the end.
> 
> Maybe, but for 4.7 (and probably 4.8) I think we should be conservative and 
> only fix the problems recently introduced.
> 
> So, in the end, I think that we should go either for Richard's initial patch 
> in this thread, possibly with a ??? comment along the lines of:
> 
> 	/* ??? If we haven't computed the alignment yet, compute it now.  Note
> 	    that, if already computed above, the alignment is that of the base
> 	    object of T so it is OK to have disregarded BITPOS above.  However,
> 	    here we are using get_object_alignment_1 which returns the precise
> 	    alignment of T so we need to account for the BITPOS adjustment.  */
> 	if (!align_computed)
> 
> or for Richard's initial patch + the removal of the MEM_REF block (which is 
> midway between Richard's initial patch and Richard's last patch).

Thus I'm preparing a patch that applies to both 4.7 and 4.8 at this
point, leaving some holes I see open (which are not to be closed in
the same way due to differences in behavior of get_object_alignment
on the 4.7 branch vs. trunk).

Bootstrap and regtest on x86_64-unknown-linux-gnu running, Eric,
can you give this a strict-align target testing on the 4.7 branch?
Martin, can you re-test this on sparc (it's slightly different again)?

I'm going to remove the MEM_REF block as a followup (we can't backport
that due to pessimizations this will cause due to different
get_object_alignment behavior - yet, unless we backport that change
as well), it's a source of wrong alignment as well, due to the
usage of MAX () - the get_object_alignment result will never shrink
alignment this way (and thus implicitely relies on attrs.align
being BITS_PER_UNIT when !align_computed).

The get_object_alignment behavior change is

2012-07-18  Richard Guenther  <rguenther@suse.de>

        * tree.h (get_object_or_type_alignment): Remove.
        * builtins.c (get_object_alignment_2): New function copied from
        get_object_alignment_1.  Take extra argument to indicate whether
        we take the address of EXP.  Rework to use type alignment 
information
        if not, and return whether the result is an approximation or not.
        (get_object_alignment_1): Wrap around get_object_alignment_2.
        (get_pointer_alignment_1): Call get_object_alignment_2 indicating
        we take the address.
        (get_object_or_type_alignment): Remove.
        * expr.c (expand_assignment): Call get_object_alignment.
        (expand_expr_real_1): Likewise.

and followups.

Thanks,
Richard.

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

	PR middle-end/55882
	* emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
	account for bitpos when computing alignment.

	* gcc.dg/torture/pr55882.c: New testcase.

Index: gcc/emit-rtl.c
===================================================================
*** gcc/emit-rtl.c	(revision 195102)
--- gcc/emit-rtl.c	(working copy)
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1839,1845 ****
  
        if (!align_computed)
  	{
! 	  unsigned int obj_align = get_object_alignment (t);
  	  attrs.align = MAX (attrs.align, obj_align);
  	}
      }
--- 1839,1850 ----
  
        if (!align_computed)
  	{
! 	  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 = MAX (attrs.align, obj_align);
  	}
      }
Index: gcc/testsuite/gcc.dg/torture/pr55882.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr55882.c	(revision 0)
--- gcc/testsuite/gcc.dg/torture/pr55882.c	(working copy)
***************
*** 0 ****
--- 1,94 ----
+ /* { dg-do run } */
+ 
+ typedef enum
+ {
+   PVT_A = 0,
+   PVT_B = 1,
+   PVT_CONFIG = 2,
+   PVT_RESERVED3 = 3,
+ } T_CR_SELECT;
+ 
+ typedef enum
+ {
+   STD_ULOGIC_0 = 0,
+   STD_ULOGIC_1 = 1,
+ } STD_ULOGIC;
+ 
+ typedef struct
+ {
+   unsigned char rtp : 3;
+   unsigned char rtn : 3;
+ } C;
+ 
+ typedef struct
+ {
+   unsigned char nd;
+   unsigned char pd;
+   unsigned char rtn;
+   unsigned char rtp;
+ } A;
+ 
+ typedef struct
+ {
+   unsigned short reserved : 14;
+   unsigned char Z_rx_enable : 2;
+   A pvt;
+ } B;
+ 
+ typedef struct
+ {
+   B cr_dsclk_q3;
+   B cr_data_q3;
+   B cr_addr_q3;
+   B cr_cmd_q3;
+   B cr_pres_q3;
+   C cr_vref_q3[6];
+   unsigned char pres_disable;
+   unsigned char pres_drive_high;
+   unsigned char c_enab_120;
+   STD_ULOGIC clk_tximp;
+   STD_ULOGIC dqs_tximp;
+   STD_ULOGIC cmd_tximp;
+   STD_ULOGIC data_tximp;
+   STD_ULOGIC dqs_rxterm;
+   STD_ULOGIC data_rxterm;
+   T_CR_SELECT cr_clk_sel;
+   unsigned char cr_clk : 5;
+   T_CR_SELECT cr_dsclk_odd_sel;
+   unsigned char cr_dsclk_odd : 5;
+   T_CR_SELECT cr_dsclk_even_sel;
+   unsigned char cr_dsclk_even : 5;
+   T_CR_SELECT cr_data_sel;
+   unsigned char cr_data : 5;
+   T_CR_SELECT cr_vref_sel;
+   unsigned char cr_vref : 5;
+   T_CR_SELECT cr_others_sel;
+   unsigned char cr_others : 5;
+ } CONFIG;
+ 
+ typedef struct
+ {
+   unsigned char enable_monitor;
+   unsigned short step_out_pointer : 12;
+   unsigned short hold_out_pointer : 12;
+   unsigned short enable_wr_dqs : 12;
+   unsigned short use_alt_rd_dqs : 12;
+   CONFIG io_buf;
+ } mystruct;
+ 
+ unsigned short __attribute__((noinline,noclone))
+ testfunction(unsigned i)
+ {
+   mystruct dmfe[8];
+   dmfe[0].use_alt_rd_dqs = 1;
+   dmfe[i].use_alt_rd_dqs = 0;
+   return dmfe[0].use_alt_rd_dqs;
+ }
+ 
+ extern void abort (void);
+ int main ()
+ {
+   if (testfunction(0) != 0) 
+     abort ();
+   return 0;
+ }

Re: [PATCH] Fix PR55882

[Richard Biener]

On Fri, 11 Jan 2013, Richard Biener wrote:

> On Thu, 10 Jan 2013, Eric Botcazou wrote:
> 
> > > The problem with this intermediate inconsistency is that ...
> > > 
> > > > That's why only the final result should matter and need be correct.
> > > 
> > > ... this can't be ensured anymore.  In some cases the inconsistency
> > > between the mem attributes and what XEXP(MEM, 0) represents can't be
> > > repaired by the later bitpos adjustments, or better it can be only be
> > > repaired by falling back to the conservative side for e.g. the alignment,
> > > because we don't store enough information in the mem attributes to recover
> > > what was lost.
> > 
> > Indeed, looking at the history of the code shows that the alignment handling 
> > via MEM_REF and get_object_alignment is an afterthought.  Originally, it was 
> > set only in a few specific cases:
> > 
> >   /* We can set the alignment from the type if we are making an object,
> >      this is an INDIRECT_REF, or if TYPE_ALIGN_OK.  */
> > 
> > and also for DECL_P and CONSTANT_CLASS_P.  That was it, so this was actually 
> > the alignment of the base and disregarding BITPOS was fine.
> 
> Yes, in these cases bitpos is zero.  It's wrong in the !align_computed
> path and possibly in the ARRAY_REF -> DECL_P path (only not as
> optimistic as it could be).
> 
> > > When briefly discussing this yesterday I suggested (without having the
> > > code in front of me) that it be best to simply set the mem attributes only
> > > at the very end, after having computed to final MEM address including all
> > > adjustments, instead of generating wrong mem attributes initially and
> > > hoping for the adjustments to make it come out right at the end.
> > 
> > Maybe, but for 4.7 (and probably 4.8) I think we should be conservative and 
> > only fix the problems recently introduced.
> > 
> > So, in the end, I think that we should go either for Richard's initial patch 
> > in this thread, possibly with a ??? comment along the lines of:
> > 
> > 	/* ??? If we haven't computed the alignment yet, compute it now.  Note
> > 	    that, if already computed above, the alignment is that of the base
> > 	    object of T so it is OK to have disregarded BITPOS above.  However,
> > 	    here we are using get_object_alignment_1 which returns the precise
> > 	    alignment of T so we need to account for the BITPOS adjustment.  */
> > 	if (!align_computed)
> > 
> > or for Richard's initial patch + the removal of the MEM_REF block (which is 
> > midway between Richard's initial patch and Richard's last patch).
> 
> Thus I'm preparing a patch that applies to both 4.7 and 4.8 at this
> point, leaving some holes I see open (which are not to be closed in
> the same way due to differences in behavior of get_object_alignment
> on the 4.7 branch vs. trunk).
> 
> Bootstrap and regtest on x86_64-unknown-linux-gnu running, Eric,
> can you give this a strict-align target testing on the 4.7 branch?
> Martin, can you re-test this on sparc (it's slightly different again)?
> 
> I'm going to remove the MEM_REF block as a followup (we can't backport
> that due to pessimizations this will cause due to different
> get_object_alignment behavior - yet, unless we backport that change
> as well), it's a source of wrong alignment as well, due to the
> usage of MAX () - the get_object_alignment result will never shrink
> alignment this way (and thus implicitely relies on attrs.align
> being BITS_PER_UNIT when !align_computed).
> 
> The get_object_alignment behavior change is
> 
> 2012-07-18  Richard Guenther  <rguenther@suse.de>
> 
>         * tree.h (get_object_or_type_alignment): Remove.
>         * builtins.c (get_object_alignment_2): New function copied from
>         get_object_alignment_1.  Take extra argument to indicate whether
>         we take the address of EXP.  Rework to use type alignment 
> information
>         if not, and return whether the result is an approximation or not.
>         (get_object_alignment_1): Wrap around get_object_alignment_2.
>         (get_pointer_alignment_1): Call get_object_alignment_2 indicating
>         we take the address.
>         (get_object_or_type_alignment): Remove.
>         * expr.c (expand_assignment): Call get_object_alignment.
>         (expand_expr_real_1): Likewise.
> 
> and followups.

I have applied the minimal fix to trunk now.  We retain the clearly
incorrect fact that the !align_computed path only ever increases
alignment (it's only ever set previously to either BITS_PER_UNIT
or via the MEM_REF block).  I will momentarily test a followup
removing that block for trunk.

Thanks,
Richard.

> Thanks,
> Richard.
> 
> 2013-01-11  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/55882
> 	* emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
> 	account for bitpos when computing alignment.
> 
> 	* gcc.dg/torture/pr55882.c: New testcase.
> 
> Index: gcc/emit-rtl.c
> ===================================================================
> *** gcc/emit-rtl.c	(revision 195102)
> --- gcc/emit-rtl.c	(working copy)
> *************** set_mem_attributes_minus_bitpos (rtx ref
> *** 1839,1845 ****
>   
>         if (!align_computed)
>   	{
> ! 	  unsigned int obj_align = get_object_alignment (t);
>   	  attrs.align = MAX (attrs.align, obj_align);
>   	}
>       }
> --- 1839,1850 ----
>   
>         if (!align_computed)
>   	{
> ! 	  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 = MAX (attrs.align, obj_align);
>   	}
>       }
> Index: gcc/testsuite/gcc.dg/torture/pr55882.c
> ===================================================================
> *** gcc/testsuite/gcc.dg/torture/pr55882.c	(revision 0)
> --- gcc/testsuite/gcc.dg/torture/pr55882.c	(working copy)
> ***************
> *** 0 ****
> --- 1,94 ----
> + /* { dg-do run } */
> + 
> + typedef enum
> + {
> +   PVT_A = 0,
> +   PVT_B = 1,
> +   PVT_CONFIG = 2,
> +   PVT_RESERVED3 = 3,
> + } T_CR_SELECT;
> + 
> + typedef enum
> + {
> +   STD_ULOGIC_0 = 0,
> +   STD_ULOGIC_1 = 1,
> + } STD_ULOGIC;
> + 
> + typedef struct
> + {
> +   unsigned char rtp : 3;
> +   unsigned char rtn : 3;
> + } C;
> + 
> + typedef struct
> + {
> +   unsigned char nd;
> +   unsigned char pd;
> +   unsigned char rtn;
> +   unsigned char rtp;
> + } A;
> + 
> + typedef struct
> + {
> +   unsigned short reserved : 14;
> +   unsigned char Z_rx_enable : 2;
> +   A pvt;
> + } B;
> + 
> + typedef struct
> + {
> +   B cr_dsclk_q3;
> +   B cr_data_q3;
> +   B cr_addr_q3;
> +   B cr_cmd_q3;
> +   B cr_pres_q3;
> +   C cr_vref_q3[6];
> +   unsigned char pres_disable;
> +   unsigned char pres_drive_high;
> +   unsigned char c_enab_120;
> +   STD_ULOGIC clk_tximp;
> +   STD_ULOGIC dqs_tximp;
> +   STD_ULOGIC cmd_tximp;
> +   STD_ULOGIC data_tximp;
> +   STD_ULOGIC dqs_rxterm;
> +   STD_ULOGIC data_rxterm;
> +   T_CR_SELECT cr_clk_sel;
> +   unsigned char cr_clk : 5;
> +   T_CR_SELECT cr_dsclk_odd_sel;
> +   unsigned char cr_dsclk_odd : 5;
> +   T_CR_SELECT cr_dsclk_even_sel;
> +   unsigned char cr_dsclk_even : 5;
> +   T_CR_SELECT cr_data_sel;
> +   unsigned char cr_data : 5;
> +   T_CR_SELECT cr_vref_sel;
> +   unsigned char cr_vref : 5;
> +   T_CR_SELECT cr_others_sel;
> +   unsigned char cr_others : 5;
> + } CONFIG;
> + 
> + typedef struct
> + {
> +   unsigned char enable_monitor;
> +   unsigned short step_out_pointer : 12;
> +   unsigned short hold_out_pointer : 12;
> +   unsigned short enable_wr_dqs : 12;
> +   unsigned short use_alt_rd_dqs : 12;
> +   CONFIG io_buf;
> + } mystruct;
> + 
> + unsigned short __attribute__((noinline,noclone))
> + testfunction(unsigned i)
> + {
> +   mystruct dmfe[8];
> +   dmfe[0].use_alt_rd_dqs = 1;
> +   dmfe[i].use_alt_rd_dqs = 0;
> +   return dmfe[0].use_alt_rd_dqs;
> + }
> + 
> + extern void abort (void);
> + int main ()
> + {
> +   if (testfunction(0) != 0) 
> +     abort ();
> +   return 0;
> + }
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend

Re: [PATCH] Fix PR55882

[Eric Botcazou]

> I have applied the minimal fix to trunk now.  We retain the clearly
> incorrect fact that the !align_computed path only ever increases
> alignment (it's only ever set previously to either BITS_PER_UNIT
> or via the MEM_REF block).  I will momentarily test a followup
> removing that block for trunk.

What do we do for the 4.7 branch?  The patch doesn't compile for it so I 
presume that there are one or more changes to be backported first?

-- 
Eric Botcazou

Re: [PATCH] Fix PR55882

[Richard Biener]

On Tue, 15 Jan 2013, Eric Botcazou wrote:

> > I have applied the minimal fix to trunk now.  We retain the clearly
> > incorrect fact that the !align_computed path only ever increases
> > alignment (it's only ever set previously to either BITS_PER_UNIT
> > or via the MEM_REF block).  I will momentarily test a followup
> > removing that block for trunk.
> 
> What do we do for the 4.7 branch?  The patch doesn't compile for it so I 
> presume that there are one or more changes to be backported first?

Not necessarily.  The following is a 4.7 variant of the patch
(on trunk get_object_alignment_1 got one extra output which
moved the align return value to by-reference).

Can you give it testing on the branch and a strict-align target?

Thanks,
Richard.

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

	PR middle-end/55882
	* emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
	account for bitpos when computing alignment.

	* gcc.dg/torture/pr55882.c: New testcase.

Index: gcc/testsuite/gcc.dg/torture/pr55882.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr55882.c	(revision 0)
--- gcc/testsuite/gcc.dg/torture/pr55882.c	(working copy)
***************
*** 0 ****
--- 1,94 ----
+ /* { dg-do run } */
+ 
+ typedef enum
+ {
+   PVT_A = 0,
+   PVT_B = 1,
+   PVT_CONFIG = 2,
+   PVT_RESERVED3 = 3,
+ } T_CR_SELECT;
+ 
+ typedef enum
+ {
+   STD_ULOGIC_0 = 0,
+   STD_ULOGIC_1 = 1,
+ } STD_ULOGIC;
+ 
+ typedef struct
+ {
+   unsigned char rtp : 3;
+   unsigned char rtn : 3;
+ } C;
+ 
+ typedef struct
+ {
+   unsigned char nd;
+   unsigned char pd;
+   unsigned char rtn;
+   unsigned char rtp;
+ } A;
+ 
+ typedef struct
+ {
+   unsigned short reserved : 14;
+   unsigned char Z_rx_enable : 2;
+   A pvt;
+ } B;
+ 
+ typedef struct
+ {
+   B cr_dsclk_q3;
+   B cr_data_q3;
+   B cr_addr_q3;
+   B cr_cmd_q3;
+   B cr_pres_q3;
+   C cr_vref_q3[6];
+   unsigned char pres_disable;
+   unsigned char pres_drive_high;
+   unsigned char c_enab_120;
+   STD_ULOGIC clk_tximp;
+   STD_ULOGIC dqs_tximp;
+   STD_ULOGIC cmd_tximp;
+   STD_ULOGIC data_tximp;
+   STD_ULOGIC dqs_rxterm;
+   STD_ULOGIC data_rxterm;
+   T_CR_SELECT cr_clk_sel;
+   unsigned char cr_clk : 5;
+   T_CR_SELECT cr_dsclk_odd_sel;
+   unsigned char cr_dsclk_odd : 5;
+   T_CR_SELECT cr_dsclk_even_sel;
+   unsigned char cr_dsclk_even : 5;
+   T_CR_SELECT cr_data_sel;
+   unsigned char cr_data : 5;
+   T_CR_SELECT cr_vref_sel;
+   unsigned char cr_vref : 5;
+   T_CR_SELECT cr_others_sel;
+   unsigned char cr_others : 5;
+ } CONFIG;
+ 
+ typedef struct
+ {
+   unsigned char enable_monitor;
+   unsigned short step_out_pointer : 12;
+   unsigned short hold_out_pointer : 12;
+   unsigned short enable_wr_dqs : 12;
+   unsigned short use_alt_rd_dqs : 12;
+   CONFIG io_buf;
+ } mystruct;
+ 
+ unsigned short __attribute__((noinline,noclone))
+ testfunction(unsigned i)
+ {
+   mystruct dmfe[8];
+   dmfe[0].use_alt_rd_dqs = 1;
+   dmfe[i].use_alt_rd_dqs = 0;
+   return dmfe[0].use_alt_rd_dqs;
+ }
+ 
+ extern void abort (void);
+ int main ()
+ {
+   if (testfunction(0) != 0) 
+     abort ();
+   return 0;
+ }
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	(revision 195201)
+++ gcc/emit-rtl.c	(working copy)
@@ -1839,7 +1839,12 @@ set_mem_attributes_minus_bitpos (rtx ref
 
       if (!align_computed)
 	{
-	  unsigned int obj_align = get_object_alignment (t);
+	  unsigned int obj_align;
+	  unsigned HOST_WIDE_INT obj_bitpos;
+	  obj_align = get_object_alignment_1 (t, &obj_bitpos);
+	  obj_bitpos = (obj_bitpos - bitpos) & (obj_align - 1);
+	  if (obj_bitpos != 0)
+	    obj_align = (obj_bitpos & -obj_bitpos);
 	  attrs.align = MAX (attrs.align, obj_align);
 	}
     }

Re: [PATCH] Fix PR55882

[Eric Botcazou]

> Not necessarily.  The following is a 4.7 variant of the patch
> (on trunk get_object_alignment_1 got one extra output which
> moved the align return value to by-reference).

OK, I obviously didn't try very hard here...

> Can you give it testing on the branch and a strict-align target?
> 
> Thanks,
> Richard.
> 
> 2013-01-15  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/55882
> 	* emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
> 	account for bitpos when computing alignment.
> 
> 	* gcc.dg/torture/pr55882.c: New testcase.

Bootstrapped/regtested on 4.7 branch for SPARC/Solaris, all clear.

-- 
Eric Botcazou

Re: [PATCH] Fix PR55882

[Richard Biener]

On Wed, 16 Jan 2013, Eric Botcazou wrote:

> > Not necessarily.  The following is a 4.7 variant of the patch
> > (on trunk get_object_alignment_1 got one extra output which
> > moved the align return value to by-reference).
> 
> OK, I obviously didn't try very hard here...
> 
> > Can you give it testing on the branch and a strict-align target?
> > 
> > Thanks,
> > Richard.
> > 
> > 2013-01-15  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR middle-end/55882
> > 	* emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
> > 	account for bitpos when computing alignment.
> > 
> > 	* gcc.dg/torture/pr55882.c: New testcase.
> 
> Bootstrapped/regtested on 4.7 branch for SPARC/Solaris, all clear.

Thanks, committed on the branch as well.

The situation with the MEM_REF block on trunk isn't all that bad,
it can't result in bogus alignment as far as I can see.  Thus I'll leave
further cleanups to when stage1 opens again.

Richard.