[Bug target/105463] New: [12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[Bug target/105463] New: [12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[acoplan at gcc dot gnu.org]

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

            Bug ID: 105463
           Summary: [12/13 Regression] MVE: Wrong code, incorrect
                    alignment assumption
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: acoplan at gcc dot gnu.org
  Target Milestone: ---

The following code is reduced from gcc.c-torture/execute/pr65369.c:

void foo (unsigned *);
void bar (const unsigned char *block)
{
  unsigned buf[4];
  __builtin_memcpy (buf +  0, block +  0, 4);
  __builtin_memcpy (buf +  1, block +  4, 4);
  __builtin_memcpy (buf +  2, block +  8, 4);
  __builtin_memcpy (buf +  3, block + 12, 4);
  foo (buf);
}

and it is miscompiled, starting in GCC 12, for MVE (with
-march=armv8.1-m.main+mve -O2). We generate:

bar:
        mov     r3, r0
        push    {lr}
        sub     sp, sp, #20
        mov     r0, sp
        vldrw.32        q3, [r3]
        vstrw.32        q3, [r0]
        bl      foo
        add     sp, sp, #20
        ldr     pc, [sp], #4

where vldrw.32 requires a 4-byte aligned address, but the source of course has
no such requirement: memcpy does not require an aligned address.

[Bug target/105463] [12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
We vectorize correctly using a 1-byte aligned load:

void __GIMPLE (ssa,guessed_local(1073741824))
bar (const unsigned char * block)
{
  vector(4) unsigned int vect__3.5;
  unsigned int buf[4];

  __BB(2,guessed_local(1073741824)):
  vect__3.5_14 = __MEM <vector(4) unsigned int, 8> ((char *
{ref-all})block_2(D));
  __MEM <vector(4) unsigned int> ((char * {ref-all})&buf) = vect__3.5_14;
  foo (&buf);
  buf ={v} _Literal (unsigned int[4]) {CLOBBER(eol)};
  return;

and expand to

(insn 9 8 10 (set (reg:V4SI 118 [ vect__3.5 ])
        (unspec:V4SI [
                (mem:V4SI (reg/v/f:SI 114 [ block ]) [0 MEM <vector(4) unsigned
int> [(char * {ref-all})block_2(D)]+0 S16 A8])
            ] UNSPEC_MISALIGNED_ACCESS)) "t.c":5:3 -1
     (nil))

which also looks OK.  'buf' is appropriately aligned:

(insn 10 9 0 (set (mem/c:V4SI (reg/f:SI 117) [0 MEM <vector(4) unsigned int>
[(char * {ref-all})&buf]+0 S16 A64])
        (reg:V4SI 118 [ vect__3.5 ])) "t.c":5:3 -1

and it's OK till the very end:

(insn 9 12 10 2 (set (reg:V4SI 28 s12 [orig:118 vect__3.5 ] [118])
        (unspec:V4SI [
                (mem:V4SI (reg:SI 3 r3 [120]) [0 MEM <vector(4) unsigned int>
[(char * {ref-all})block_2(D)]+0 S16 A8])
            ] UNSPEC_MISALIGNED_ACCESS)) "t.c":5:3 4951
{*movmisalignv4si_mve_load}
     (expr_list:REG_DEAD (reg:SI 3 r3 [120])
        (nil)))

so it's a bug in the machine description somehow.

[Bug target/105463] [12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
(define_insn "*movmisalign<mode>_mve_load"
  [(set (match_operand:MVE_VLD_ST 0 "s_register_operand"                       
         "=w")
        (unspec:MVE_VLD_ST [(match_operand:MVE_VLD_ST 1
"neon_permissive_struct_operand" " Ux")]
         UNSPEC_MISALIGNED_ACCESS))]
  "((TARGET_HAVE_MVE && VALID_MVE_SI_MODE (<MODE>mode))
    || (TARGET_HAVE_MVE_FLOAT && VALID_MVE_SF_MODE (<MODE>mode)))
   && !BYTES_BIG_ENDIAN && unaligned_access"
  "vldr<V_sz_elem1>.<V_sz_elem>\t%q0, %E1"
  [(set_attr "type" "mve_load")]
)

-mno-unaligned-access "fixes" it, the above simply lets the improper(?) load
through.

[Bug target/105463] [12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[acoplan at gcc dot gnu.org]

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

--- Comment #3 from Alex Coplan <acoplan at gcc dot gnu.org> ---
Note with -O3 GCC 11 also has the problem, it's just that with GCC 12
vectorization was enabled by default at -O2. GCC 10 seems unaffected.

[Bug target/105463] [11/12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.0                        |11.4
            Summary|[12/13 Regression] MVE:     |[11/12/13 Regression] MVE:
                   |Wrong code, incorrect       |Wrong code, incorrect
                   |alignment assumption        |alignment assumption

[Bug target/105463] [11/12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[rearnsha at gcc dot gnu.org]

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

--- Comment #4 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
Vector loads on MVE need to be lane-sized aligned.

I think the movmisalign pattern for MVE needs to emit VLDRB.8 regardless of the
mode, rather than an inner-mode sized access.  Fortunately, for little-endian
this makes no difference to the internal representation.

[Bug target/105463] [11/12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[acoplan at gcc dot gnu.org]

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

--- Comment #5 from Alex Coplan <acoplan at gcc dot gnu.org> ---
FWIW (which is probably not much) with -O3 the problem started with
r11-3681-g29c650cd899496c4f9bc069d03d0d7ecfb632176 .

commit 29c650cd899496c4f9bc069d03d0d7ecfb632176
Author: Dennis Zhang <dennis.zh@live.com>
Date:   Tue Oct 6 16:53:46 2020

    arm: Enable MVE SIMD modes for vectorization

But I suppose the underlying issue is likely still latent (to do with the
movmisalign pattern as richi mentioned above).

[Bug target/105463] [11/12/13 Regression] MVE: Wrong code, incorrect alignment assumption

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:6a116728e27c4da65d84483c0e75561a7479d4d5

commit r13-417-g6a116728e27c4da65d84483c0e75561a7479d4d5
Author: Richard Earnshaw <rearnsha@arm.com>
Date:   Wed May 11 13:08:40 2022 +0100

    arm: correctly handle misaligned MEMs on MVE [PR105463]

    Vector operations in MVE must be aligned to the element size, so if we
    are asked for a misaligned move in a wider mode we must recast it to a
    form suitable for the known alignment (larger elements have better
    address offset ranges, so there is some advantage to using wider
    element sizes if possible).  Whilst fixing this, also rework the
    predicates used for validating operands - the Neon predicates are
    not right for MVE.

    gcc/ChangeLog:

            PR target/105463
            * config/arm/mve.md (*movmisalign<mode>_mve_store): Use
            mve_memory_operand.
            (*movmisalign<mode>_mve_load): Likewise.
            * config/arm/vec-common.md (movmisalign<mode>): Convert to
generator
            form...
            (@movmisalign<mode>): ... thus.  Use generic predicates and then
            rework operands if they are not valid.  For MVE rework to a
            narrower element size if the alignment is not high enough.

[Bug target/105463] [11/12 Regression] MVE: Wrong code, incorrect alignment assumption

[rearnsha at gcc dot gnu.org]

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

Richard Earnshaw <rearnsha at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rearnsha at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-05-13
            Summary|[11/12/13 Regression] MVE:  |[11/12 Regression] MVE:
                   |Wrong code, incorrect       |Wrong code, incorrect
                   |alignment assumption        |alignment assumption

--- Comment #7 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
Fixed on master branch

[Bug target/105463] [11/12 Regression] MVE: Wrong code, incorrect alignment assumption

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug target/105463] [11/12 Regression] MVE: Wrong code, incorrect alignment assumption

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r12-8737-gde1ba234311b935b1a38d512e57329d4b6e8354d
Author: Richard Earnshaw <rearnsha@arm.com>
Date:   Wed May 11 13:08:40 2022 +0100

    arm: correctly handle misaligned MEMs on MVE [PR105463]

    Vector operations in MVE must be aligned to the element size, so if we
    are asked for a misaligned move in a wider mode we must recast it to a
    form suitable for the known alignment (larger elements have better
    address offset ranges, so there is some advantage to using wider
    element sizes if possible).  Whilst fixing this, also rework the
    predicates used for validating operands - the Neon predicates are
    not right for MVE.

    gcc/ChangeLog:

            PR target/105463
            * config/arm/mve.md (*movmisalign<mode>_mve_store): Use
            mve_memory_operand.
            (*movmisalign<mode>_mve_load): Likewise.
            * config/arm/vec-common.md (movmisalign<mode>): Convert to
generator
            form...
            (@movmisalign<mode>): ... thus.  Use generic predicates and then
            rework operands if they are not valid.  For MVE rework to a
            narrower element size if the alignment is not high enough.

    (cherry picked from commit 6a116728e27c4da65d84483c0e75561a7479d4d5)

[Bug target/105463] [11 Regression] MVE: Wrong code, incorrect alignment assumption

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r11-10231-gac1fce509ddd99e825073b3a9eab5911ac3dc454
Author: Richard Earnshaw <rearnsha@arm.com>
Date:   Wed May 11 13:08:40 2022 +0100

    arm: correctly handle misaligned MEMs on MVE [PR105463]

    Vector operations in MVE must be aligned to the element size, so if we
    are asked for a misaligned move in a wider mode we must recast it to a
    form suitable for the known alignment (larger elements have better
    address offset ranges, so there is some advantage to using wider
    element sizes if possible).  Whilst fixing this, also rework the
    predicates used for validating operands - the Neon predicates are
    not right for MVE.

    gcc/ChangeLog:

            PR target/105463
            * config/arm/mve.md (*movmisalign<mode>_mve_store): Use
            mve_memory_operand.
            (*movmisalign<mode>_mve_load): Likewise.
            * config/arm/vec-common.md (movmisalign<mode>): Convert to
generator
            form...
            (@movmisalign<mode>): ... thus.  Use generic predicates and then
            rework operands if they are not valid.  For MVE rework to a
            narrower element size if the alignment is not high enough.

    (cherry picked from commit 6a116728e27c4da65d84483c0e75561a7479d4d5)

[Bug target/105463] [11 Regression] MVE: Wrong code, incorrect alignment assumption

[rearnsha at gcc dot gnu.org]

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

Richard Earnshaw <rearnsha at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #10 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
Fixed on all relevant branches