[Bug target/54252] New: [Neon] Bad alignment code generated for Neon loads

[Bug target/54252] New: [Neon] Bad alignment code generated for Neon loads

[eric.batut at allegorithmic dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

             Bug #: 54252
           Summary: [Neon] Bad alignment code generated for Neon loads
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: eric.batut@allegorithmic.com


Created attachment 28009
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28009
Small repro case

Using gcc trunk at rev 190381, compiled with the Android NDK r8b build-gcc.sh
script (so an arm-linux-androideabi target) and the command line below, the
attached repro case does not compile, and spits the following error messages:

Erics-Mac:src batut$
/Users/batut/android-ndk-r8b/toolchains/arm-linux-androideabi-4.8.0/prebuilt/darwin-x86/bin/arm-linux-androideabi-gcc
-mfloat-abi=hard -mfpu=vfp -mfpu=neon -marm -O1 -c test.c
/var/folders/nr/l7qlwr295379gn7tqyv61jx80000gn/T//cc9BgV0B.s: Assembler
messages:
/var/folders/nr/l7qlwr295379gn7tqyv61jx80000gn/T//cc9BgV0B.s:29: Error: bad
alignment -- `vld1.32 {d16},[r3:128]!'
/var/folders/nr/l7qlwr295379gn7tqyv61jx80000gn/T//cc9BgV0B.s:32: Error: bad
alignment -- `vld1.32 {d7},[r2:128]'

The assembly code generated is:
algNEON:
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    @ link register save eliminated.
    ldr r3, .L2
.LPIC0:
    add r3, pc, r3
    ldr r2, .L2+4
    ldr r2, [r3, r2]
    mov r3, r2
    vld1.32 {d16}, [r3:128]!  <= Offending load
    vld1.32 {d6}, [r3:64]
    add r2, r2, #16
    vld1.32 {d7}, [r2:128]    <= Offending load
    vadd.f32    d16, d0, d16
    vmov.f32    d0, #0.0  @ v2sf
    vmla.f32    d0, d16, d6[0]
    vmls.f32    d0, d16, d6[1]
    vmla.f32    d0, d16, d7[0]
    vmls.f32    d0, d16, d7[1]
    bx  lr

This does not happen at -O0.
This also happens with gcc 4.7.1.


arm-linux-androideabi-gcc -v
Using built-in specs.
COLLECT_GCC=/Users/batut/android-ndk-r8b/toolchains/arm-linux-androideabi-4.8.0/prebuilt/darwin-x86/bin/arm-linux-androideabi-gcc
COLLECT_LTO_WRAPPER=/Users/batut/android-ndk-r8b/toolchains/arm-linux-androideabi-4.8.0/prebuilt/darwin-x86/bin/../libexec/gcc/arm-linux-androideabi/4.8.0/lto-wrapper
Target: arm-linux-androideabi
Configured with:
/Users/batut/android-ndk-r8b/src/build/../gcc/gcc-4.8.0/configure
--prefix=/tmp/ndk-batut/build/toolchain/prefix --target=arm-linux-androideabi
--host=x86_64-apple-darwin --build=x86_64-apple-darwin --with-gnu-as
--with-gnu-ld --enable-languages=c,c++
--with-gmp=/tmp/ndk-batut/build/toolchain/temp-install
--with-mpfr=/tmp/ndk-batut/build/toolchain/temp-install
--with-mpc=/tmp/ndk-batut/build/toolchain/temp-install --without-ppl
--without-cloog --disable-libssp --enable-threads --disable-nls
--disable-libmudflap --disable-libgomp --disable-libstdc__-v3
--disable-sjlj-exceptions --disable-shared --disable-tls --disable-libitm
--with-float=soft --with-fpu=vfp --with-arch=armv5te --enable-target-optspace
--enable-initfini-array --disable-nls
--prefix=/tmp/ndk-batut/build/toolchain/prefix
--with-sysroot=/tmp/ndk-batut/build/toolchain/prefix/sysroot
--with-binutils-version=2.22 --with-mpfr-version=2.4.1 --with-mpc-version=0.8.1
--with-gmp-version=5.0.5 --with-gcc-version=4.8.0 --with-gdb-version=7.3.x
--disable-bootstrap --disable-libquadmath --disable-plugin --with-arch=armv5te
--program-transform-name='s&^&arm-linux-androideabi-&'
Thread model: posix
gcc version 4.8.0 20120814 (experimental) (GCC)

[Bug target/54252] [Neon] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-15
                 CC|                            |ramana at gcc dot gnu.org
     Ever Confirmed|0                           |1

[Bug target/54252] [Neon] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #1 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-08-15 11:00:53 UTC ---
hmmm MEM_SIZE for the offending load appears to be 32 bytes. Something is fishy
here. 

 vld1.32 {d16}, [r3:128]!  <= Offending load

[Bug target/54252] [Neon] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #2 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-08-15 11:18:12 UTC ---
(In reply to comment #1)
> hmmm MEM_SIZE for the offending load appears to be 32 bytes. Something is fishy
> here. 
> 
>  vld1.32 {d16}, [r3:128]!  <= Offending load

(insn 13 12 0 (set (reg:V2SF 112 [ D.14059 ])
        (unspec:V2SF [
                (mem/u/c:V2SF (reg:SI 125) [0 MEM[(const float32_t[8] *)&consts
+ 16B]+0 S32 A128])
            ] UNSPEC_VLD1))
/home/ramrad01/sources/fsf/build-sanity/gcc/include/arm_neon.h:7908 -1
     (nil))


As you can see it appears to think that the size of the load is what's in
MEM_SIZE . However that wouldn't be the case here as we are only loading
V2SFmode values but the underlying address is the address of what is an array
of 8 float32_t values. 

This thoroughly untested appears to work around the problem but I don't like
this one bit. 

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c    (revision 190409)
+++ gcc/config/arm/arm.c    (working copy)
@@ -17320,7 +17320,10 @@
        instruction (for some alignments) as an aid to the memory subsystem
        of the target.  */
     align = MEM_ALIGN (x) >> 3;
-    memsize = MEM_SIZE (x);
+    if (MEM_SIZE_KNOWN_P (x))
+      memsize = MIN (MEM_SIZE (x), GET_MODE_SIZE (GET_MODE (x)));
+    else
+      memsize = 0;

     /* Only certain alignment specifiers are supported by the hardware.  */
     if (memsize == 32 && (align % 32) == 0)


regards,
Ramana

[Bug target/54252] [Neon] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #3 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-08-15 13:03:41 UTC ---
That ain't the root cause and that patch should really not be applied. 

The problem really is in neon_dereference_pointer where we expand such builtins
- that's the one that is creating mem_refs of float32_t [8] rather than a
float32_t[2] .

I'll take a look.

ramana

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.7.2
            Summary|[4.7 Regression] Bad        |[4.7/4.8 Regression] Bad
                   |alignment code generated    |alignment code generated
                   |for Neon loads              |for Neon loads

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #5 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-08-28 12:20:29 UTC ---
Looking into this now.

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #6 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-08-30 10:17:08 UTC ---
Author: ramana
Date: Thu Aug 30 10:17:04 2012
New Revision: 190800

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190800
Log:
Fix PR target/54252

2012-08-29  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
        Richard Earnshaw  <richard.earnshaw@arm.com>

    PR target/54252
    * config/arm/arm.c (neon_dereference_pointer): Adjust nelems by
    element size. Use elem_type from the formal parameter. New parameter
    fcode.
    (neon_expand_args): Adjust call to neon_dereference_pointer.



Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #7 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-08-30 10:21:59 UTC ---
Fixed now I believe on trunk.

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[eric.batut at allegorithmic dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #8 from Eric Batut <eric.batut at allegorithmic dot com> 2012-08-30 15:52:20 UTC ---
The original bug instance is fixed on trunk (rev 190803).
I had what I think is another instance of the same bug, where the error message
is "alignment of array elements is greater than element size", and this is also
fixed by rev 190803.

(In reply to comment #7)
> Fixed now I believe on trunk.

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[ramrad01 at arm dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #9 from ramrad01 at arm dot com 2012-08-30 16:48:12 UTC ---
On 30/08/12 16:52, eric.batut at allegorithmic dot com wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252
>
> --- Comment #8 from Eric Batut <eric.batut at allegorithmic dot com> 2012-08-30 15:52:20 UTC ---
> The original bug instance is fixed on trunk (rev 190803).
> I had what I think is another instance of the same bug, where the error message
> is "alignment of array elements is greater than element size", and this is also
> fixed by rev 190803.


Good to hear that this fixes the original bug instance on trunk. I 
intend to do a backport to 4.7 next week if there are no more problems 
reported with this.


Thanks,
Ramana

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

--- Comment #10 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-09-07 09:32:00 UTC ---
Author: ramana
Date: Fri Sep  7 09:31:54 2012
New Revision: 191059

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191059
Log:
Fix PR target/54252

2012-09-07  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

    Backport from mainline.
    2012-08-29  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
                   Richard Earnshaw  <richard.earnshaw@arm.com>

    PR target/54252
    * config/arm/arm.c (neon_dereference_pointer): Adjust nelems by
    element size. Use elem_type from the formal parameter. New parameter
    fcode.
    (neon_expand_args): Adjust call to neon_dereference_pointer.




Modified:
    branches/gcc-4_7-branch/   (props changed)
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/arm/arm.c

Propchange: branches/gcc-4_7-branch/
            ('svn:mergeinfo' modified)

[Bug target/54252] [4.7/4.8 Regression] Bad alignment code generated for Neon loads

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54252

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

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

--- Comment #11 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-07 10:28:05 UTC ---
Fixed.