[Bug middle-end/67718] New: [aarch64] long double incorrect code for copysign

[Bug middle-end/67718] New: [aarch64] long double incorrect code for copysign

[thanm at google dot com]

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

            Bug ID: 67718
           Summary: [aarch64] long double incorrect code for copysign
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: thanm at google dot com
  Target Milestone: ---

Created attachment 36392
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36392&action=edit
reduced test case (C++)

Compiling the attached example with g++ at -O2 or -O1 results in incorrect code
(-O0 looks ok). This is with gcc trunk aarch64-linux-android target; problem is
also present in 4.9.  Source being compiled looks like:

/// begin
union IEEEl2bits {
  long double e;
  struct {
    unsigned long manl  :64;
    unsigned long manh  :48;
    unsigned int  exp   :15;
    unsigned int  sign  :1;
  } bits;
};

long double
copysignl(long double x, long double y)
{
  union IEEEl2bits ux, uy;
  ux.e = x;
  uy.e = y;
  ux.bits.sign = uy.bits.sign;
  return (ux.e);
}
/// end

Here is -O2 assembly:

        ushr    d1, d1, 63
        fmov    x0, d0
        fmov    x1, v0.d[1]
        fmov    d0, x0
        fmov    x2, d1
        bfi     x1, x2, 63, 1
        fmov    v0.d[1], x1
        ret

Note that the first instruction is sourcing d1 (incorrect) -- this needs to
read q0 in order for correct semantics.

I am not a gcc expert, but I'm guessing that the problem is happening in the
reload phase. Prior to reload the RTL looks ok:

(insn 10 9 7 2 (set (reg:DI 81 [ D.3189 ])
        (lshiftrt:DI (subreg:DI (reg:TI 33 v1 [ y ]) 8)
            (const_int 63 [0x3f])))
bionic/libm/upstream-freebsd/lib/msun/src/s_copysignl.c:40 512
{*aarch64_lshr_sisd_or_int_di3}
     (expr_list:REG_DEAD (reg:TI 33 v1 [ y ])
        (nil)))

and then after reload this gets turned into

(insn 10 9 7 2 (set (reg:DI 33 v1 [orig:81 D.3189 ] [81])
        (lshiftrt:DI (reg:DI 33 v1 [ y+8 ])
            (const_int 63 [0x3f])))
bionic/libm/upstream-freebsd/lib/msun/src/s_copysignl.c:40 512
{*aarch64_lshr_sisd_or_int_di3}
     (nil))

which doesn't look right -- we seem to have lost the "TI", which I take to be
the bug.

[Bug target/67718] [aarch64] long double incorrect code for copysign

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|middle-end                  |target

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
What is happening is the subreg is being assumed it is lower part of the
register rather than the upper part.  Really this subreg should have declared
as invalid for v1 as there is no way to do scalar for the upper part of the
register.

This makes this a target issue rather than a middle-end one.

Also by the way using __builtin_copysignl should generate better code
generation anyways.

[Bug target/67718] [aarch64] long double incorrect code for copysign

[thanm at google dot com]

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

--- Comment #2 from Than McIntosh <thanm at google dot com> ---

>>using __builtin_copysignl should generate better code generation anyways.

Tried replacing the guts of the function with a call to __builtin_copysignl --
the bug is still present.

Any recommendations on someone in the extended GCC community who could help me
triage/analyze the bug? I have worked with register allocators before, but not
with gcc's; I could use some guidance.

[Bug target/67718] [aarch64] long double incorrect code for copysign

[ramana at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2015-09-30
                 CC|                            |ramana at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #3 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> ---
With the latest build for aarch64-none-linux-gnu of 4.9 and trunk that I had
lying around, I see with -O2 and with and without a variety of tuning options,
which looks a whole load better than the original bug report.

copysignl:
        fmov    x2, d0
        fmov    x1, v1.d[1]
        fmov    x3, v0.d[1]
        fmov    d0, x2
        lsr     x1, x1, 63
        bfi     x3, x1, 63, 1
        fmov    v0.d[1], x3
        ret
        .size   copysignl, .-copysignl
        .ident  "GCC: 4.9.4 20150924 (prerelease)"

I am traveling currently and can't dig any further but I remember something
being fixed in this pattern with respect to early clobbers- Aha PR61633 may be
your friend.

Can you check if you have the fix for PR61633 in your 4.9 based tree ? 

In the future please report with command line options and the exact upstream
source revision used to create the issue.

regards
Ramana

[Bug target/67718] [aarch64] long double incorrect code for copysign

[thanm at google dot com]

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

Than McIntosh <thanm at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |INVALID

--- Comment #4 from Than McIntosh <thanm at google dot com> ---
Thanks for the response.

You are right: the code generated by 4.9 and trunk gcc is correct -- after
going back and rebuilding and retesting I am seeing the same code you posted.
I'm not sure what I did wrong the first time; sorry for the spam / noise /
false alarm. I will mark the bug as INVALID...

The specific copy of gcc I'm working with (which as you pointed out I should
have included in the original report) is the Android platform/NDK copy of gcc
4.9, which is a variant based on the
svn://gcc.gnu.org/svn/gcc/branches/google/gcc-4_9 branch with the addition of
some changes back-ported from trunk.

I can reproduce the same problem by building directly from the gcc
google/gcc-4_9 branch (no Android changes), so it looks as though that is where
things went off the tracks.