Re: [patch] Respect known misaligned addresses in set_mem_attributes_minus_bitpos

[Richard Guenther <richard dot guenther at gmail dot com>]

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
>