Patchwork Respect known misaligned addresses in set_mem_attributes_minus_bitpos

login
register
mail settings
Submitter Ulrich Weigand
Date Dec. 30, 2010, 12:22 p.m.
Message ID <201012301222.oBUCM7GP029710@d06av02.portsmouth.uk.ibm.com>
Download mbox | patch
Permalink /patch/76990/
State New
Headers show

Comments

Ulrich Weigand - Dec. 30, 2010, 12:22 p.m.
Richard Guenther wrote:

> The correct way to fix this is IMHO to make set_mem_attributes_minus_bitpos
> not get initial alignment from the mode (but assert the mem-attrs are not
> set yet in that function).  At least if I understand the problem correctly.
> 
> Ulrich promised to do this a while back ...

Sorry for the long delay; I finally found some time to work on this.

The patch below changes set_mem_attributes_minus_bitpos to not rely
on the implicit defaulting rules in the MEM_ accessor macros.  Instead,
they are used only if memory attributes are in fact present.  If not,
deriving defaults for size and alignment from the MEM's mode are
open-coded in this function.

The overall behaviour should be identical to before, except for the one
case we wanted to change: if set_mem_attributes_minus_bitpos is called
with an expression (not a type) describing an object, the alignment is
determined solely from that expression; default alignment of the mode
is ignored, even on STRICT_ALIGNMENT targets.

This fixes the problem I was originally seeing on a (modified) SPU target
(and in fact generates fully identical code to what I was getting with my
back-end work-around).  Ramana, does this fix the ARM problem as well?


There may be additional changes one could possibly do:

- Is set_mem_attributes_minus_bitpos ever called on a MEM that already has
  attributes?  If not, this set of defaults could go away ...

- Is there really any case where using the mode's *size* is necessary (i.e.
  where we don't already get the size from the type)?

- If we don't get an expression, but just a type, the rules when to use
  the type's alignment seem somewhat complex to me ...   If this could
  be simplified so we can *always* use the type alignment, they all the
  STRICT_ALIGNMENT special case could maybe go there?

But I guess all this could be follow-on work.

Tested on s390x-linux, powerpc64-linux and spu-elf with no regressions.

OK for mainline?

Bye,
Ulrich


ChangeLog:

	* emit-rtl.c (set_mem_attributes_minus_bitpos): Explicitly derive
	default values from MEM mode if no memory attributes are present.
	Do not use mode alignment, even on STRICT_ALIGNMENT targets, when
	called with an expression (not a type).
Richard Guenther - Dec. 30, 2010, 12:49 p.m.
On Thu, Dec 30, 2010 at 1:22 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Richard Guenther wrote:
>
>> The correct way to fix this is IMHO to make set_mem_attributes_minus_bitpos
>> not get initial alignment from the mode (but assert the mem-attrs are not
>> set yet in that function).  At least if I understand the problem correctly.
>>
>> Ulrich promised to do this a while back ...
>
> Sorry for the long delay; I finally found some time to work on this.
>
> The patch below changes set_mem_attributes_minus_bitpos to not rely
> on the implicit defaulting rules in the MEM_ accessor macros.  Instead,
> they are used only if memory attributes are in fact present.  If not,
> deriving defaults for size and alignment from the MEM's mode are
> open-coded in this function.
>
> The overall behaviour should be identical to before, except for the one
> case we wanted to change: if set_mem_attributes_minus_bitpos is called
> with an expression (not a type) describing an object, the alignment is
> determined solely from that expression; default alignment of the mode
> is ignored, even on STRICT_ALIGNMENT targets.
>
> This fixes the problem I was originally seeing on a (modified) SPU target
> (and in fact generates fully identical code to what I was getting with my
> back-end work-around).  Ramana, does this fix the ARM problem as well?
>
>
> There may be additional changes one could possibly do:
>
> - Is set_mem_attributes_minus_bitpos ever called on a MEM that already has
>  attributes?  If not, this set of defaults could go away ...

I think we shouldn't call it if there are already mem attributes, but
to be safe just
leave the code in for now (can you add a FIXME comment?).

> - Is there really any case where using the mode's *size* is necessary (i.e.
>  where we don't already get the size from the type)?

I don't think so (but the function needs some overall cleanup anyway, your
changes look reasonable for stage3).

> - If we don't get an expression, but just a type, the rules when to use
>  the type's alignment seem somewhat complex to me ...   If this could
>  be simplified so we can *always* use the type alignment, they all the
>  STRICT_ALIGNMENT special case could maybe go there?

I think we should deprecate the case where we get a type in 't'.  And indeed
if we get a type we should just use TYPE_ALIGN - mode alignment doesn't
honor alignment attributes, so you'd get wrong code again on strict align
targets.

> But I guess all this could be follow-on work.

Indeed.

> Tested on s390x-linux, powerpc64-linux and spu-elf with no regressions.
>
> OK for mainline?

Ok with some FIXME comments according to the issues you mention added.

Thanks,
Richard.

> Bye,
> Ulrich
>
>
> ChangeLog:
>
>        * emit-rtl.c (set_mem_attributes_minus_bitpos): Explicitly derive
>        default values from MEM mode if no memory attributes are present.
>        Do not use mode alignment, even on STRICT_ALIGNMENT targets, when
>        called with an expression (not a type).
>
> Index: gcc-head/gcc/emit-rtl.c
> ===================================================================
> --- gcc-head.orig/gcc/emit-rtl.c
> +++ gcc-head/gcc/emit-rtl.c
> @@ -1540,11 +1540,11 @@ void
>  set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp,
>                                 HOST_WIDE_INT bitpos)
>  {
> -  alias_set_type alias = MEM_ALIAS_SET (ref);
> -  tree expr = MEM_EXPR (ref);
> -  rtx offset = MEM_OFFSET (ref);
> -  rtx size = MEM_SIZE (ref);
> -  unsigned int align = MEM_ALIGN (ref);
> +  alias_set_type alias;
> +  tree expr = NULL;
> +  rtx offset = NULL_RTX;
> +  rtx size = NULL_RTX;
> +  unsigned int align = BITS_PER_UNIT;
>   HOST_WIDE_INT apply_bitpos = 0;
>   tree type;
>
> @@ -1580,6 +1580,27 @@ set_mem_attributes_minus_bitpos (rtx ref
>       && TREE_CODE (type) != COMPLEX_TYPE)
>     MEM_SCALAR_P (ref) = 1;
>
> +  /* Default values from pre-existing memory attributes if present.  */
> +  if (MEM_ATTRS (ref))
> +    {
> +      expr = MEM_EXPR (ref);
> +      offset = MEM_OFFSET (ref);
> +      size = MEM_SIZE (ref);
> +      align = MEM_ALIGN (ref);
> +    }
> +
> +  /* Otherwise, default values from the mode of the MEM reference.  */
> +  else if (GET_MODE (ref) != BLKmode)
> +    {
> +      /* Respect mode size.  */
> +      size = GEN_INT (GET_MODE_SIZE (GET_MODE (ref)));
> +
> +      /* Respect mode alignment for STRICT_ALIGNMENT targets if T is a type;
> +         if T is an object, always compute the object alignment below.  */
> +      if (STRICT_ALIGNMENT && TYPE_P (t))
> +       align = GET_MODE_ALIGNMENT (GET_MODE (ref));
> +    }
> +
>   /* We can set the alignment from the type if we are making an object,
>      this is an INDIRECT_REF, or if TYPE_ALIGN_OK.  */
>   if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type))
>
> --
>  Dr. Ulrich Weigand
>  GNU Toolchain for Linux on System z and Cell BE
>  Ulrich.Weigand@de.ibm.com
>
Ramana Radhakrishnan - Jan. 4, 2011, 11:43 a.m.
> This fixes the problem I was originally seeing on a (modified) SPU target
> (and in fact generates fully identical code to what I was getting with my
> back-end work-around).  Ramana, does this fix the ARM problem as well?

Sorry been away on vacation. I believe your committed version does because some of the recent test reports seem to indicate this has been fixed. Updating and testing latest version of trunk now.

cheers
Ramana

Patch

Index: gcc-head/gcc/emit-rtl.c
===================================================================
--- gcc-head.orig/gcc/emit-rtl.c
+++ gcc-head/gcc/emit-rtl.c
@@ -1540,11 +1540,11 @@  void
 set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp,
 				 HOST_WIDE_INT bitpos)
 {
-  alias_set_type alias = MEM_ALIAS_SET (ref);
-  tree expr = MEM_EXPR (ref);
-  rtx offset = MEM_OFFSET (ref);
-  rtx size = MEM_SIZE (ref);
-  unsigned int align = MEM_ALIGN (ref);
+  alias_set_type alias;
+  tree expr = NULL;
+  rtx offset = NULL_RTX;
+  rtx size = NULL_RTX;
+  unsigned int align = BITS_PER_UNIT;
   HOST_WIDE_INT apply_bitpos = 0;
   tree type;
 
@@ -1580,6 +1580,27 @@  set_mem_attributes_minus_bitpos (rtx ref
       && TREE_CODE (type) != COMPLEX_TYPE)
     MEM_SCALAR_P (ref) = 1;
 
+  /* Default values from pre-existing memory attributes if present.  */
+  if (MEM_ATTRS (ref))
+    {
+      expr = MEM_EXPR (ref);
+      offset = MEM_OFFSET (ref);
+      size = MEM_SIZE (ref);
+      align = MEM_ALIGN (ref);
+    }
+
+  /* Otherwise, default values from the mode of the MEM reference.  */
+  else if (GET_MODE (ref) != BLKmode)
+    {
+      /* Respect mode size.  */
+      size = GEN_INT (GET_MODE_SIZE (GET_MODE (ref)));
+
+      /* Respect mode alignment for STRICT_ALIGNMENT targets if T is a type;
+         if T is an object, always compute the object alignment below.  */
+      if (STRICT_ALIGNMENT && TYPE_P (t))
+	align = GET_MODE_ALIGNMENT (GET_MODE (ref));
+    }
+
   /* We can set the alignment from the type if we are making an object,
      this is an INDIRECT_REF, or if TYPE_ALIGN_OK.  */
   if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type))