[Bug tree-optimization/96075] New: bogus alignment for negative step grouped access

[Bug tree-optimization/96075] New: bogus alignment for negative step grouped access

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

            Bug ID: 96075
           Summary: bogus alignment for negative step grouped access
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

double x[1024], y[1024];

void __attribute__((noipa)) boo()
{
  for (int i = 0; i < 512; ++i)
    {
      x[2*i] = y[2*(511-i)];
      x[2*i+1] = y[2*(511-i)];
    }
}

int
main ()
{   
  for (int i = 0; i < 1024; ++i)
    {
      x[i] = 0;
      y[i] = i;
      __asm__ volatile ("");
    }

  boo ();
  for (int i = 0; i < 1024; ++i)
    if (x[i] != y[2*(511 - i/2)])
      abort ();

  return 0;
}


segfaults because we emit

boo:
.LFB8:
        .cfi_startproc
        xorl    %eax, %eax
        .p2align 4,,10
        .p2align 3
.L21:
        movapd  y+8168(%rax), %xmm0
        movq    %rax, %rdx

where this is obviously misaligned.

[Bug tree-optimization/96075] [8/9/10/11 Regression] bogus alignment for negative step grouped access

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
                 CC|                            |marxin at gcc dot gnu.org
            Summary|bogus alignment for         |[8/9/10/11 Regression]
                   |negative step grouped       |bogus alignment for
                   |access                      |negative step grouped
                   |                            |access
   Target Milestone|---                         |8.5
           Keywords|                            |needs-bisection
      Known to fail|                            |10.1.1, 8.4.0, 9.3.0
      Known to work|                            |10.1.0, 7.5.0, 8.3.0, 9.2.0

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Regressed somewhere on all branches but GCC 7.  Bisecting on the GCC 10 branch
appreciated.

[Bug tree-optimization/96075] [8/9/10/11 Regression] bogus alignment for negative step grouped access

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |10.1.1
      Known to fail|10.1.1                      |

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Correction, the GCC 10 branch seems unaffected.

[Bug tree-optimization/96075] [8/9/10/11 Regression] bogus alignment for negative step grouped access

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
                 CC|                            |rsandifo at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2020-07-06

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
So we end up calling get_negative_load_store_type for this group which seems
to only handle contiguous accesses but this one is single element interleaving
aka contiguous with gap.

vect_supportable_dr_alignment returns dr_aligned which is seemingly OK for

#(Data Ref: 
#  bb: 3 
#  stmt: _4 = y[_2];
#  ref: y[_2];
#  base_object: y;
#  Access function 0: {1022, +, -2}_1

but then as said the vls type is computed as VMAT_CONTIGUOUS_REVERSE where
we eventually do

  if (memory_access_type == VMAT_CONTIGUOUS_DOWN
      || memory_access_type == VMAT_CONTIGUOUS_REVERSE)
    offset = size_int (-TYPE_VECTOR_SUBPARTS (vectype) + 1);

invalidating that alignment...

  /* If this is a backward running DR then first access in the larger
     vectype actually is N-1 elements before the address in the DR.
     Adjust misalign accordingly.  */
  if (tree_int_cst_sgn (drb->step) < 0)
    /* PLUS because STEP is negative.  */
    misalignment += ((TYPE_VECTOR_SUBPARTS (vectype) - 1)
                     * TREE_INT_CST_LOW (drb->step));

looks a bit odd since this does not offset by N-1 elements but N-1
"steps" (where a step is two elements in this case).  Here's probably
where the problem resides.  On the GCC 7 branch we see

  /* If this is a backward running DR then first access in the larger
     vectype actually is N-1 elements before the address in the DR.
     Adjust misalign accordingly.  */
  if (tree_int_cst_sgn (step) < 0)
    { 
      tree offset = ssize_int (TYPE_VECTOR_SUBPARTS (vectype) - 1);
      /* DR_STEP(dr) is the same as -TYPE_SIZE of the scalar type,
         otherwise we wouldn't be here.  */
      offset = fold_build2 (MULT_EXPR, ssizetype, offset, step);
      /* PLUS because STEP was negative.  */
      misalign = size_binop (PLUS_EXPR, misalign, offset);
    }

and mind that comment about DR_STEP isn't true ...

diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c
index 2462276e7c2..959c2d3378f 100644
--- a/gcc/tree-vect-data-refs.c
+++ b/gcc/tree-vect-data-refs.c
@@ -1109,7 +1109,7 @@ vect_compute_data_ref_alignment (vec_info *vinfo,
dr_vec_info *dr_info)
   if (tree_int_cst_sgn (drb->step) < 0)
     /* PLUS because STEP is negative.  */
     misalignment += ((TYPE_VECTOR_SUBPARTS (vectype) - 1)
-                    * TREE_INT_CST_LOW (drb->step));
+                    * -TREE_INT_CST_LOW (TYPE_SIZE_UNIT (TREE_TYPE
(vectype))));

   unsigned int const_misalignment;
   if (!known_misalignment (misalignment, vect_align_c, &const_misalignment))

fixes that.

[Bug tree-optimization/96075] [8/9/10/11 Regression] bogus alignment for negative step grouped access

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #4 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> So we end up calling get_negative_load_store_type for this group which seems
> to only handle contiguous accesses but this one is single element
> interleaving
> aka contiguous with gap.
> 
> vect_supportable_dr_alignment returns dr_aligned which is seemingly OK for
> 
> #(Data Ref: 
> #  bb: 3 
> #  stmt: _4 = y[_2];
> #  ref: y[_2];
> #  base_object: y;
> #  Access function 0: {1022, +, -2}_1
Are you sure we support this?  I think…

>   /* If this is a backward running DR then first access in the larger
>      vectype actually is N-1 elements before the address in the DR.
>      Adjust misalign accordingly.  */
>   if (tree_int_cst_sgn (step) < 0)
>     { 
>       tree offset = ssize_int (TYPE_VECTOR_SUBPARTS (vectype) - 1);
>       /* DR_STEP(dr) is the same as -TYPE_SIZE of the scalar type,
>          otherwise we wouldn't be here.  */
…really was the assumption for negative steps at one time,
and I'm not sure off-hand when/if that changed.

(Of course, it might be that one of my patches changed it.)

[Bug tree-optimization/96075] [8/9/10/11 Regression] bogus alignment for negative step grouped access

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 6 Jul 2020, rsandifo at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075
> 
> --- Comment #4 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #3)
> > So we end up calling get_negative_load_store_type for this group which seems
> > to only handle contiguous accesses but this one is single element
> > interleaving
> > aka contiguous with gap.
> > 
> > vect_supportable_dr_alignment returns dr_aligned which is seemingly OK for
> > 
> > #(Data Ref: 
> > #  bb: 3 
> > #  stmt: _4 = y[_2];
> > #  ref: y[_2];
> > #  base_object: y;
> > #  Access function 0: {1022, +, -2}_1
> Are you sure we support this?  I think…
> 
> >   /* If this is a backward running DR then first access in the larger
> >      vectype actually is N-1 elements before the address in the DR.
> >      Adjust misalign accordingly.  */
> >   if (tree_int_cst_sgn (step) < 0)
> >     { 
> >       tree offset = ssize_int (TYPE_VECTOR_SUBPARTS (vectype) - 1);
> >       /* DR_STEP(dr) is the same as -TYPE_SIZE of the scalar type,
> >          otherwise we wouldn't be here.  */
> …really was the assumption for negative steps at one time,
> and I'm not sure off-hand when/if that changed.
> 
> (Of course, it might be that one of my patches changed it.)

Yeah - me too.  We still don't seem to support those for
interleaving [single element] groups but we let them slip
through for SLP ...

That said, the patch shouldn't make the situation worse at least ;)

[Bug tree-optimization/96075] [8/9/10/11 Regression] bogus alignment for negative step grouped access

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:dccbf1e2a6e544f71b4a5795f0c79015db019fc3

commit r11-1851-gdccbf1e2a6e544f71b4a5795f0c79015db019fc3
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Jul 6 16:26:50 2020 +0200

    tree-optimization/96075 - fix bogus misalignment calculation

    This fixes bogus misalignment calculation for negative steps
    since an assertion a previous comment indicated no longer holds:

          /* DR_STEP(dr) is the same as -TYPE_SIZE of the scalar type,
             otherwise we wouldn't be here.  */

    Thus the following replaces DR_STEP by -TYPE_SIZE.

    2020-07-06  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/96075
            * tree-vect-data-refs.c (vect_compute_data_ref_alignment): Use
            TYPE_SIZE_UNIT of the vector component type instead of DR_STEP
            for the misalignment calculation for negative step.

            * gcc.dg/vect/slp-46.c: New testcase.

[Bug tree-optimization/96075] [8/9/10 Regression] bogus alignment for negative step grouped access

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |11.0
            Summary|[8/9/10/11 Regression]      |[8/9/10 Regression] bogus
                   |bogus alignment for         |alignment for negative step
                   |negative step grouped       |grouped access
                   |access                      |

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk sofar, surely latent on the GCC 10 branch.

[Bug tree-optimization/96075] [8/9/10 Regression] bogus alignment for negative step grouped access

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
On the 10 branch we "optimize" the load as

  _19 = MEM[(double *)vectp_y.4_7 + 8B];
  _17 = {0, _19};

possibly because of the gap.

[Bug tree-optimization/96075] [8/9/10 Regression] bogus alignment for negative step grouped access

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:7d535ca86a548b76384f3687e1d46677cb652bdb

commit r10-8433-g7d535ca86a548b76384f3687e1d46677cb652bdb
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Jul 6 16:26:50 2020 +0200

    tree-optimization/96075 - fix bogus misalignment calculation

    This fixes bogus misalignment calculation for negative steps
    since an assertion a previous comment indicated no longer holds:

          /* DR_STEP(dr) is the same as -TYPE_SIZE of the scalar type,
             otherwise we wouldn't be here.  */

    Thus the following replaces DR_STEP by -TYPE_SIZE.

    2020-07-06  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/96075
            * tree-vect-data-refs.c (vect_compute_data_ref_alignment): Use
            TYPE_SIZE_UNIT of the vector component type instead of DR_STEP
            for the misalignment calculation for negative step.

            * gcc.dg/vect/slp-46.c: New testcase.

    (cherry picked from commit dccbf1e2a6e544f71b4a5795f0c79015db019fc3)

[Bug tree-optimization/96075] [8/9 Regression] bogus alignment for negative step grouped access

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #10 from Martin Liška <marxin at gcc dot gnu.org> ---
Likely started with r8-568-gf9f69dd651b2f103.

after:

.L7:
        movapd  y+8176(%rax), %xmm0
        movq    %rax, %rdx
        subq    $16, %rax
        negq    %rdx
        cmpq    $-8192, %rax
        unpcklpd        %xmm0, %xmm0
        movaps  %xmm0, x(%rdx)
        jne     .L7
        xorl    %edx, %edx
        .p2align 4,,10
        .p2align 3

before:

        movapd  (%rax), %xmm0
        addq    $16, %rdx
        leaq    -16(%rax), %rcx
        unpcklpd        %xmm0, %xmm0
        movaps  %xmm0, -16(%rdx)
        cmpq    $y, %rax
        jne     .L11
        xorl    %edx, %edx
        .p2align 4,,10
        .p2align 3

[Bug tree-optimization/96075] [8/9 Regression] bogus alignment for negative step grouped access

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #10)
> Likely started with r8-568-gf9f69dd651b2f103.

Very unlikely (IVOPTS runs after vectorization)

[Bug tree-optimization/96075] [8/9 Regression] bogus alignment for negative step grouped access

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:53d76fe758a3e8e0c1f049e75030278427a8fe5d

commit r9-9094-g53d76fe758a3e8e0c1f049e75030278427a8fe5d
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Jul 6 16:26:50 2020 +0200

    tree-optimization/96075 - fix bogus misalignment calculation

    This fixes bogus misalignment calculation for negative steps
    since an assertion a previous comment indicated no longer holds:

          /* DR_STEP(dr) is the same as -TYPE_SIZE of the scalar type,
             otherwise we wouldn't be here.  */

    Thus the following replaces DR_STEP by -TYPE_SIZE.

    2020-07-06  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/96075
            * tree-vect-data-refs.c (vect_compute_data_ref_alignment): Use
            TYPE_SIZE_UNIT of the vector component type instead of DR_STEP
            for the misalignment calculation for negative step.

            * gcc.dg/vect/slp-46.c: New testcase.

    (cherry picked from commit dccbf1e2a6e544f71b4a5795f0c79015db019fc3)

[Bug tree-optimization/96075] [8 Regression] bogus alignment for negative step grouped access

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

Christophe Lyon <clyon at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |clyon at gcc dot gnu.org

--- Comment #13 from Christophe Lyon <clyon at gcc dot gnu.org> ---
The new test fails on gcc-9 on aarch64:

FAIL: gcc.dg/vect/slp-46.c -flto -ffat-lto-objects  scan-tree-dump-times vect
"vectorizing stmts using SLP" 2
FAIL: gcc.dg/vect/slp-46.c scan-tree-dump-times vect "vectorizing stmts using
SLP" 2

The log says "pattern found 0 times"

[Bug tree-optimization/96075] [8 Regression] bogus alignment for negative step grouped access

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #14 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 3 Dec 2020, clyon at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075
> 
> Christophe Lyon <clyon at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |clyon at gcc dot gnu.org
> 
> --- Comment #13 from Christophe Lyon <clyon at gcc dot gnu.org> ---
> The new test fails on gcc-9 on aarch64:
> 
> FAIL: gcc.dg/vect/slp-46.c -flto -ffat-lto-objects  scan-tree-dump-times vect
> "vectorizing stmts using SLP" 2
> FAIL: gcc.dg/vect/slp-46.c scan-tree-dump-times vect "vectorizing stmts using
> SLP" 2
> 
> The log says "pattern found 0 times"

Hmm, trunk has { xfail vect_load_lanes } there, but the GCC 10 branch also
misses that.  Can you check if the GCC 10 branch also sees this FAIL and
if adding the XFAIL helps?

[Bug tree-optimization/96075] [8 Regression] bogus alignment for negative step grouped access

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #15 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Yes, the test fails on gcc-10 too.

I tried adding xfail vect_load_lanes in gcc-9, and it correctly makes the test
XFAIL.

[Bug tree-optimization/96075] [8 Regression] bogus alignment for negative step grouped access

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #16 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:0f50805bb3b0924bab94bd85203370703af50f26

commit r10-9120-g0f50805bb3b0924bab94bd85203370703af50f26
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Dec 4 11:39:44 2020 +0100

    tree-optimization/96075 - adjust testcase

    This adds an XFAIL when load-lanes can be used.

    2020-12-04  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/96075
            * gcc.dg/vect/slp-46.c: Add XFAIL for load-lanes.

[Bug tree-optimization/96075] [8 Regression] bogus alignment for negative step grouped access

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:d910ca0493455ffa84d4d934957cda1cb1588a25

commit r9-9098-gd910ca0493455ffa84d4d934957cda1cb1588a25
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Dec 4 11:39:44 2020 +0100

    tree-optimization/96075 - adjust testcase

    This adds an XFAIL when load-lanes can be used.

    2020-12-04  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/96075
            * gcc.dg/vect/slp-46.c: Add XFAIL for load-lanes.

    (cherry picked from commit 0f50805bb3b0924bab94bd85203370703af50f26)

[Bug tree-optimization/96075] [8 Regression] bogus alignment for negative step grouped access

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96075

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|8.5                         |9.4
                 CC|                            |jakub at gcc dot gnu.org
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The GCC 8 branch is being closed, fixed in GCC 9.4.