[Bug target/55147] New: x86: wrong code for 64-bit load

[Bug target/55147] New: x86: wrong code for 64-bit load

[mans at mansr dot com]

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

             Bug #: 55147
           Summary: x86: wrong code for 64-bit load
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: mans@mansr.com


> cat foo.c
unsigned foo(unsigned long long *p, int i)
{
    return __builtin_bswap64(p[i]);
}

> gcc-4.8.0 -m32 -O1 -S foo.c
foo:
        movl    8(%esp), %edx
        movl    4(%esp), %eax
        movl    (%eax,%edx,8), %edx
        movl    4(%eax,%edx,8), %eax
        bswap   %eax
        ret

Note the first movl overwriting the index register %edx.

[Bug target/55147] x86: wrong code for 64-bit load

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-10-31
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |uros at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-31 15:37:49 UTC ---
--- gcc/config/i386/i386.md.jj    2012-10-30 09:01:15.000000000 +0100
+++ gcc/config/i386/i386.md    2012-10-31 16:25:13.986916767 +0100
@@ -12688,7 +12688,14 @@ (define_insn_and_split "*bswapdi2_double
    (set (match_dup 0)
       (bswap:SI (match_dup 3)))]
 {
-  split_double_mode (DImode, &operands[0], 2, &operands[0], &operands[2]);
+  split_double_mode (DImode, &operands[0], 1, &operands[0], &operands[2]);
+  if (MEM_P (operands[1])
+      && reg_overlap_mentioned_p (operands[2], operands[1]))
+    {
+      emit_insn (gen_rtx_SET (VOIDmode, operands[0], XEXP (operands[1], 0)));
+      operands[1] = change_address (operands[1], VOIDmode, operands[0]);
+    }
+  split_double_mode (DImode, &operands[1], 1, &operands[1], &operands[3]);

   if (REG_P (operands[0]) && REG_P (operands[1]))
     {

can fix the reg overlap problem between address of operands[1] and high part of
operands[0].  That said, I wonder what is the advantage of having bswapdi2
patter on i?86 at all that the generic expand_doubleword_bswap can't handle.

[Bug target/55147] x86: wrong code for 64-bit load

[ubizjak at gmail dot com]

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

--- Comment #2 from Uros Bizjak <ubizjak at gmail dot com> 2012-10-31 15:47:46 UTC ---
> can fix the reg overlap problem between address of operands[1] and high part of
> operands[0].  That said, I wonder what is the advantage of having bswapdi2
> patter on i?86 at all that the generic expand_doubleword_bswap can't handle.

If generic code creates similar assembly, then I'm all for removing this
pattern.

[Bug target/55147] x86: wrong code for 64-bit load

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-31 16:07:11 UTC ---
For the testcase from this PR it creates better assembly actually (compared to
with the #c1 patch, without that it is both longer and wrong).  That is because
when bswapdi is split too late, nothing optimizes the fact that only 32 bits of
the result are used.

For
unsigned long long
f1 (unsigned long long *p, int i)
{
  return __builtin_bswap64 (p[i]);
}

unsigned long long
f2 (unsigned long long p)
{
  return __builtin_bswap64 (p);
}

void
f3 (unsigned long long *p, int i, unsigned long long q)
{
  p[i] = __builtin_bswap64 (q);
}

void
f4 (unsigned long long *p, int i, unsigned long long *q)
{
  p[i] = __builtin_bswap64 (q[i]);
}

it creates the same number of insns/same quality (just slightly different RA
decisions/scheduling) for f1-f3, but for f4 without bswapdi2 it creates
slightly worse code (with bswapdi2 f4 needs just one call saved register,
without it two, supposedly because both bswap insns are scheduled together.

[Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load

[ubizjak at gmail dot com]

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

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.8.0
            Summary|x86: wrong code for 64-bit  |[4.8 Regression] x86: wrong
                   |load                        |code for 64-bit load

--- Comment #4 from Uros Bizjak <ubizjak at gmail dot com> 2012-11-01 09:27:21 UTC ---
(In reply to comment #3)

> it creates the same number of insns/same quality (just slightly different RA
> decisions/scheduling) for f1-f3, but for f4 without bswapdi2 it creates
> slightly worse code (with bswapdi2 f4 needs just one call saved register,
> without it two, supposedly because both bswap insns are scheduled together.

We can live with that.

I have also checked that removing the pattern doesn't degrade TARGET_MOVBE, the
reason for their existence is PR53227.

Also, a regression from 4.7.

[Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-01 09:44:22 UTC ---
Created attachment 28589
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28589
gcc48-pr55147.patch

So like this?  Or do you want to merge the bswap{si,di}2 expanders using SWI48
iterator too?  That would make i386.md tiny bit shorter, but would make
gen_bswapdi2 longer (as the compiler can't figure out that for DImode (thus
TARGET_64BIT) TARGET_BSWAP is always true).  Perhaps I could do
  else if (<MODE>mode == DImode || TARGET_BSWAP)
so that at least optimized gcc builds would optimize it away.

[Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load

[ubizjak at gmail dot com]

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

--- Comment #6 from Uros Bizjak <ubizjak at gmail dot com> 2012-11-01 09:48:50 UTC ---
(In reply to comment #5)
> Created attachment 28589 [details]
> gcc48-pr55147.patch
> 
> So like this?  Or do you want to merge the bswap{si,di}2 expanders using SWI48
> iterator too?  That would make i386.md tiny bit shorter, but would make
> gen_bswapdi2 longer (as the compiler can't figure out that for DImode (thus
> TARGET_64BIT) TARGET_BSWAP is always true).  Perhaps I could do
>   else if (<MODE>mode == DImode || TARGET_BSWAP)
> so that at least optimized gcc builds would optimize it away.

No, your proposed patch is OK and pre-approved for mainline SVN.

[Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-02 08:03:05 UTC ---
Author: jakub
Date: Fri Nov  2 08:03:02 2012
New Revision: 193090

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193090
Log:
    PR target/55147
    * config/i386/i386.md (bswapdi2): Limit to TARGET_64BIT.
    (*bswapdi2_doubleword): Removed.

    * gcc.target/i386/pr55147.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr55147.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
    trunk/gcc/testsuite/ChangeLog

[Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-07 10:15:41 UTC ---
Fixed.