Re: [RFC PATCH] set_mem_attributes alignment and ARRAY_REF

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

On Wed, Oct 8, 2008 at 4:51 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> While looking at code generated by various gcc.target/i386/aes*.c testcases,
> I've noticed practically all MEMs have A8, while one would expect A128.
> It turns out set_mem_attributes uses TYPE_ALIGN for INDIRECT_REFs, but
> for ARRAY_REF it would derive alignment from it only if the index is
> constant.  That seems to be very poor job.
> I don't know if all ARRAY_REFs are supposed to be always aligned to
> TYPE_ALIGN (TREE_TYPE (array_ref)), or not.  If yes, they can/should be
> handled like INDIRECT_REF for alignment.  If not, the following patch
> at least improves all the aes*.c testcases, by computing alignment
> for ARRAY_REF <DECL_P, index>, but perhaps more can be done (say, if t2
> isn't DECL_P, but say COMPONENT_REF or INDIRECT_REF, we could use minimum of
> that object's alignment and off_align the patch below already computes.
>
> E.g. on aesimc.c this patch changes:
>       movdqu  src1(%rax), %xmm0
>       aesimc  %xmm0, %xmm0
>       movdqu  %xmm0, resdst(%rax)
>       movdqu  src1+16(%rax), %xmm0
>       aesimc  %xmm0, %xmm0
> ...
> into:
>       aesimc  src1(%rax), %xmm0
>       movdqa  %xmm0, resdst(%rax)
>       aesimc  src1+16(%rax), %xmm0
>       movdqa  %xmm0, resdst+16(%rax)
> (both src1 and resdst are __m128i arrays, %rax is a multiple of 16).
> For some other aes*.c tests it just changes lots of movdqu insns into
> movdqa.
>
> Before I bootstrap/regtest it, what are your opinions about this?

I agree with your findings.  Note that it may be worth factoring
out bits to share code with builtins.c:get_pointer_alignment.

Richard.

> 2008-10-08  Jakub Jelinek  <jakub@redhat.com>
>
>        * emit-rtl.c (set_mem_attributes_minus_bitpos): Compute alignment
>        from ARRAY_REF even when off_tree isn't constant.
>
> --- gcc/emit-rtl.c.jj   2008-09-15 09:36:26.000000000 +0200
> +++ gcc/emit-rtl.c      2008-10-08 16:14:08.000000000 +0200
> @@ -1652,6 +1652,7 @@ set_mem_attributes_minus_bitpos (rtx ref
>          /* We can't modify t, because we use it at the end of the
>             function.  */
>          tree t2 = t;
> +         HOST_WIDE_INT off_align = 0;
>
>          do
>            {
> @@ -1659,6 +1660,19 @@ set_mem_attributes_minus_bitpos (rtx ref
>              tree low_bound = array_ref_low_bound (t2);
>              tree unit_size = array_ref_element_size (t2);
>
> +             if (host_integerp (unit_size, 1))
> +               {
> +                 HOST_WIDE_INT ioff = tree_low_cst (unit_size, 1);
> +                 HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT;
> +
> +                 if (t == t2)
> +                   off_align = aoff;
> +                 else
> +                   off_align = MIN (off_align, aoff);
> +               }
> +             else
> +               off_align = BITS_PER_UNIT;
> +
>              /* We assume all arrays have sizes that are a multiple of a byte.
>                 First subtract the lower bound, if any, in the type of the
>                 index, then convert to sizetype and multiply by the size of
> @@ -1691,6 +1705,15 @@ set_mem_attributes_minus_bitpos (rtx ref
>                  offset = GEN_INT (ioff);
>                  apply_bitpos = bitpos;
>                }
> +             else if (off_align > BITS_PER_UNIT)
> +               {
> +                 if (DECL_ALIGN (t2) < (unsigned HOST_WIDE_INT) off_align)
> +                   off_align = DECL_ALIGN (t2);
> +                 if (align < (unsigned HOST_WIDE_INT) off_align
> +                     && (unsigned HOST_WIDE_INT) off_align
> +                        == (unsigned int) off_align)
> +                   align = off_align;
> +               }
>            }
>          else if (TREE_CODE (t2) == COMPONENT_REF)
>            {
>
>        Jakub
>