[Bug target/52684] New: glibc long double math tests fail on sparc 64-bit

[Bug target/52684] New: glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

             Bug #: 52684
           Summary: glibc long double math tests fail on sparc 64-bit
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: davem@gcc.gnu.org


When built with gcc-4.7, several glibc long double math
tests fail on 64-bit sparc.

This might be more fallout from the rewrite of the sparc
movcc patterns, but that's just a hunch.

[Bug target/52684] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

davem at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-03-23
     Ever Confirmed|0                           |1

[Bug target/52684] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #1 from davem at gcc dot gnu.org 2012-04-29 20:48:49 UTC ---
Created attachment 27263
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27263
Testcase distilled from glibc's math/libm-test.inc compare_float_internal()

This is distilled from glibc's math/libm-test.inc:compare_float_internal() when
built for long double.

The problem is that, with -ffloat-store, the compiler eliminates the stores
into the stack that setup the stack slots for the arguments passed into
_Qp_div().

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot
                   |                            |gnu.org
   Target Milestone|---                         |4.7.1
            Summary|glibc long double math      |[4.7 regression] glibc long
                   |tests fail on sparc 64-bit  |double math tests fail on
                   |                            |sparc 64-bit

--- Comment #2 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-04-29 22:09:24 UTC ---
> This is distilled from glibc's math/libm-test.inc:compare_float_internal() when
> built for long double.
> 
> The problem is that, with -ffloat-store, the compiler eliminates the stores
> into the stack that setup the stack slots for the arguments passed into
> _Qp_div().

Using -ffloat-store is a little strange on the SPARC, but the bug is
nevertheless quite annoying.  Is it the improved RTL DSE pass?

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #3 from davem at gcc dot gnu.org 2012-05-01 22:27:11 UTC ---
Sadly, the bug is even more severe.

Even something as simple as:

long double f(long double a, long double b)
{
   return a + b;
}

will be miscompiled with -O1 -ffloat-store on 64-bit.  The problem
is that the call instructions emitted lack the reg use notes so that
the compiler can see the stack frame inputs to the libcall.  Strangely,
the notes are created properly on 32-bit.

Then DSE thinks that the stack stores can be eliminated and proceeds
to kill those insns off.

Also, code generation is significantly better with -ffloat-store
enabled.  If you don't believe me, see for yourself.

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #4 from davem at gcc dot gnu.org 2012-05-01 22:44:08 UTC ---
Ok, I see why 32-bit works.  On 32-bit we use real libfuncs in the optabs, so
the compiler can see all the typing information and emit the proper information
to stick into the CALL_INSN_FUNCTION_USAGE lists.

But when we emit soft TFmode libcalls by hand, as on 64-bit, this information
is lost.  So DSE has nothing to work with and can't see that the temporary
stack slots are inputs to the libcall.

We'll have to tack on our own reg use notes by hand in functions like
emit_soft_tfmode_libcall() and sparc_emit_float_lib_cmp().

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #5 from davem at gcc dot gnu.org 2012-05-01 23:12:46 UTC ---
Strange, I added code to tack the function usage information onto
TFmode libcalls, but DSE still removes the stack slot stores :-/

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #6 from davem at gcc dot gnu.org 2012-05-02 07:01:24 UTC ---
Created attachment 27283
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27283
Simplest test case.  Build with -O1 -ffloat-store -m64 on sparc

This is the most simplest test case which generates a long double soft-fp
libcall on sparc64, and DSE erroneously eliminates the stores that setup the
arguments.

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #7 from davem at gcc dot gnu.org 2012-05-02 07:01:52 UTC ---
Created attachment 27284
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27284
DSE debugging dump of t1.c testcase.

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #8 from davem at gcc dot gnu.org 2012-05-02 07:23:56 UTC ---
I can't see how the current DSE code can properly handle this case, and I'm
surprised this doesn't trigger more often.

The outgoing args are stored to offsets of FRAME_POINTER_REGNUM, even
though this is before reload.  This causes DSE to set ->frame_related

However I can't see anything that takes into account that these locations in
the frame are read by the CALL_INSN.

There is some weird commentary in the description of the insn_info->frame_read
field in dse.c which talks about how "the stores related to outgoing arguments
are stack pointer based and thus deemed of non-constant base in this pass" but
as explained above that is simply not true here.

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #9 from davem at gcc dot gnu.org 2012-05-02 08:09:46 UTC ---
Ok, after further study, I think I'm going to propose to fix this in DSE as
follows.

When we see a CALL in dse.c:scan_insn(), if FRAME_POINTER_REGNUM ==
ARG_POINTER_REGNUN then we will set insn_info->frame_read to true.

A quick test shows that this is sufficient to avoid the erroneous store
removals.

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #10 from davem at gcc dot gnu.org 2012-05-03 05:18:11 UTC ---
Created attachment 27298
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27298
Proposed fix.

It turns out to in the end be a sparc specific problem.

This makes sure, in the sparc backend, that we mark the MEM_EXPR of a MEM
addressable if we pass it directly into a libcall.

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #11 from davem at gcc dot gnu.org 2012-05-03 22:19:42 UTC ---
Author: davem
Date: Thu May  3 22:19:35 2012
New Revision: 187120

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187120
Log:
Fix long double float miscompilations on sparc 64-bit.

    PR target/52684
    * config/sparc/sparc.c (emit_soft_tfmode_libcall): If we pass a
    MEM directly into a libcall, mark it's MEM_EXPR as addressable.
    (sparc_emit_float_lib_cmp): Likewise.

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

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

--- Comment #12 from davem at gcc dot gnu.org 2012-05-03 22:34:41 UTC ---
Author: davem
Date: Thu May  3 22:34:34 2012
New Revision: 187124

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187124
Log:
Fix long double float miscompilations on sparc 64-bit.

    PR target/52684
    * config/sparc/sparc.c (emit_soft_tfmode_libcall): If we pass a
    MEM directly into a libcall, mark it's MEM_EXPR as addressable.
    (sparc_emit_float_lib_cmp): Likewise.

Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/sparc/sparc.c

[Bug target/52684] [4.7 regression] glibc long double math tests fail on sparc 64-bit

[davem at gcc dot gnu.org]

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

davem at gcc dot gnu.org changed:

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

--- Comment #13 from davem at gcc dot gnu.org 2012-05-03 22:54:33 UTC ---
Fixed in mainline and the 4.7 branch.