| Message ID | 201012301222.oBUCM7GP029710@d06av02.portsmouth.uk.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 >
> 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
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))