Bug 39501

Summary: -O -ffinite-math-only gets min(x,y) optimization wrong for soft-float on arm-*-gnueabi
Product: gcc Reporter: Martin Guy <martinwguy>
Component: targetAssignee: Richard Earnshaw <rearnsha>
Status: RESOLVED FIXED    
Severity: normal CC: gcc-bugs, kurt, OdinsHorse, ramana.r, ramana, rearnsha, tbm, toolchain
Priority: P2 Keywords: wrong-code
Version: 4.3.3   
Target Milestone: 4.3.4   
Host: Target: arm-linux-gnueabi
Build: Known to work:
Known to fail: 4.1.2, 4.2.4, 4.3.3 Last reconfirmed: 2009-03-19 16:53:01

Description Martin Guy 2009-03-19 13:31:10 UTC
The following fragment when compiled -O -ffinite-math-only on arm-linux-gnueabi should print 0.000000 but with gcc-4.1.2, 4.2.4, 4.3.3 it prints 99999.000000

#include <stdio.h>

#define test_min(x,y)   ((x) >  (y) ? (y) : (x))

int
main (void)
{
        static float data [1];
        float min = 99999.0 ;

        min = test_min (min, data[0]) ;

        printf("min = %f\n", min);

        return min != 0.0 ;
}

This only happens for floats, not doubles. The main difference in the asm is that without -ffinite-math-only (working) it goes
        mov     r0, r4
        ldr     r1, .L6+4
        bl      __aeabi_fcmplt
        cmp     r0, #0
        ldr     r3, .L6+4
        moveq   r4, r3
        mov     r0, r4
while with -ffinite-math-only (broken) it goes
        ldr     r5, .L5+4
        mov     r0, r4
        mov     r1, r5
        bl      __aeabi_fcmple
        cmp     r0, #0
        movgt   r4, r5
        mov     r0, r4
I guess that should be moveq.

This makes libvorbis fail on arm-linux-gnueabi when using softfloat.
See https://trac.xiph.org/ticket/1526 for a longer code example failing on min() and max().
Comment 1 Ramana Radhakrishnan 2009-03-19 15:53:38 UTC
Adding self to CC list - 

mainline is also broken. The only difference in mainline is that we generate a movle instead of movgt.

It should indeed be a moveq instead of a movle.

cheers
Ramana
Comment 2 Ramana Radhakrishnan 2009-03-19 16:05:25 UTC
Or get rid of the cmp. The Runtime ABI suggests that the Z,N,C flags are set for the result of the comparison. If that is true then the second cmp is unnecessary.

Table 5 section 4.1.2 of the ARM Runtime ABI suggests this.


Comment 3 Martin Guy 2009-03-19 16:29:58 UTC
ramana:
I think you'll find the flags are only set for the 3-way comparisons.
__aeabi_cmple just returns 0 or 1
"Use for C <=" in the table means the C language, not the carry flag.

If you can find where the error is in the GCC source, that'd be great.
Comment 4 Ramana Radhakrishnan 2009-03-19 16:49:09 UTC
(In reply to comment #3)
> ramana:
> I think you'll find the flags are only set for the 3-way comparisons.
> __aeabi_cmple just returns 0 or 1
> "Use for C <=" in the table means the C language, not the carry flag.
> If you can find where the error is in the GCC source, that'd be great.

It was pointed out that I was looking at the wrong function in the runtime ABI - so I take that back. I was referring to the void __aeabi_cfcmple(float, float) variant rather than the fcmple that's used in this case. 

So if you were to use the cfcmple variants (which gcc can't at the moment) the extra cmp might be removed.


Comment 5 Richard Earnshaw 2009-03-19 16:53:01 UTC
Also affects all other EABI target builds.

THe bug is in movsfcc (and movdfcc) which have not been corrected to account for the libcall comparisons returning a bool value in the EABI.

I'm currently testing a fix
Comment 6 Richard Earnshaw 2009-03-19 17:02:00 UTC
Correction: it doesn't affect movdfcc since that only matches on hard-float targets.
Comment 7 Andrew Pinski 2009-03-19 18:17:56 UTC
*** Bug 39507 has been marked as a duplicate of this bug. ***
Comment 8 Martin Guy 2009-03-29 17:49:24 UTC
A bit more info on the real-world cases that this bug bites.

1) libvorbis's testsuite (only in trunk) fails:

    svn co http://svn.xiph.org/trunk/vorbis
    cd vorbis; ./autogen.sh; ./configure; make
    test/test

complains that "max_abs (0.000000) too small."; it should say "ok"

It can be fixed by using "./configure CFLAGS=-fno-finite-math-only"

(Both of these utilities' build scripts append your CFLAGS settings to their own default ones.)

2) LAME -V0 outputs silence

    wget http://downloads.sourceforge.net/lame/lame-398-2.tar.gz
    tar xzf lame-398-2.tar.gz
    cd lame-398-2
    ./configure && make
    wget http://martinwguy.co.uk/martin/test/Happy.wav
    frontend/lame --nohist -V0 Happy.wav Happy.mp3
    mpg123 -w Happy2.wav Happy.mp3
    # Or frontend/lame --quiet --decode Happy.mp3 Happy2.wav
    sox Happy2.wav -e stat | grep 'Maximum amplitude'

should say something like maxamp = 0.574310, not 0.000000

It can be fixed by using
        "./configure CFLAGS=-fno-finite-math-only"
and also with
        "./configure CFLAGS="-fno-schedule-insns -fno-schedule-insns2"
though this does not fix the libvorbis issue.
Comment 9 Richard Earnshaw 2009-04-04 10:37:23 UTC
Subject: Bug 39501

Author: rearnsha
Date: Sat Apr  4 10:37:10 2009
New Revision: 145534

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=145534
Log:
	PR target/39501
	* arm.md (movsfcc): Disable if not TARGET_HARD_FLOAT.
	* testsuite/gcc.c-torture/execute/pr39501.c: New file.
	* testsuite/gcc.c-torture/execute/pr39501.x: New file.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr39501.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr39501.x
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.md

Comment 10 Martin Guy 2009-04-04 11:23:31 UTC
Is that *it*? Disable movsfcc completely without finding the cause of the specific failure?
No wonder the generated ARM code is bigger, slower and buggier with every release!
Comment 11 Martin Guy 2009-04-04 11:50:29 UTC
Sorry about that. I've now seen
http://gcc.gnu.org/ml/gcc-patches/2009-04/msg00300.html

    M
Comment 12 Steven Bosscher 2009-04-04 11:53:10 UTC
Re. comment #10: And who are you funding, Martin Guy, to improve GCC for ARM? Or are you just another user who expects magic for free?
Comment 13 Steven Bosscher 2009-04-04 12:00:21 UTC
Re. comment #10: I don't know about the "buggier" and "slower" parts.

But for "bigger" I know we have benchmarks that say otherwise. e.g. CSiBE for arm-elf (http://www.inf.u-szeged.hu/csibe/s-arm.php) and CSiBE for arm-linux (http://www.inf.u-szeged.hu/csibe/s-arm-linux.php, unfortunately stopped running in 2006 due to -- you've guessed it -- lack of funding).

Obviously there's no denying that GCC still is too far behind most commercial compilers for ARM...
Comment 14 Richard Earnshaw 2009-04-04 12:25:19 UTC
Subject: Bug 39501

Author: rearnsha
Date: Sat Apr  4 12:25:06 2009
New Revision: 145537

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=145537
Log:
	PR target/39501
	* arm.md (movsfcc): Disable if not TARGET_HARD_FLOAT.
	* testsuite/gcc.c-torture/execute/pr39501.c: New file.
	* testsuite/gcc.c-torture/execute/pr39501.x: New file.

Added:
    branches/gcc-4_4-branch/gcc/testsuite/gcc.c-torture/execute/pr39501.c
    branches/gcc-4_4-branch/gcc/testsuite/gcc.c-torture/execute/pr39501.x
Modified:
    branches/gcc-4_4-branch/gcc/ChangeLog
    branches/gcc-4_4-branch/gcc/config/arm/arm.md

Comment 15 Richard Earnshaw 2009-05-16 22:25:17 UTC
Subject: Bug 39501

Author: rearnsha
Date: Sat May 16 22:24:59 2009
New Revision: 147623

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=147623
Log:
	PR target/39501
	* arm.md (movsfcc): Disable if not TARGET_HARD_FLOAT.
	* testsuite/gcc.c-torture/execute/pr39501.c: New file.
	* testsuite/gcc.c-torture/execute/pr39501.x: New file.

Added:
    branches/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/pr39501.c
    branches/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/pr39501.x
Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/config/arm/arm.md

Comment 16 Ramana Radhakrishnan 2010-12-03 11:04:06 UTC
Isn't this now fixed ? 

Ramana
Comment 17 Martin Guy 2010-12-03 14:46:28 UTC
Sort of. The cause of the bug was never found, and the workaround is to disable an instruction.  It might be worth trying enabling movsfcc in current GCC and re-running the tests, since the conditional execution stuff in the middle end was rewritten between 4.3 and 4.4 if I remember correctly, so the actual bug in the middle end may have gone away with that rewrite.
Comment 18 Ramana Radhakrishnan 2014-04-28 14:02:33 UTC
fixed.