[Bug target/57623] New: BEXTR intrinsic has memory operands switched around (fails to compile code)

[Bug target/57623] New: BEXTR intrinsic has memory operands switched around (fails to compile code)

[sgunderson at bigfoot dot com]

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

            Bug ID: 57623
           Summary: BEXTR intrinsic has memory operands switched around
                    (fails to compile code)
           Product: gcc
           Version: 4.8.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: sgunderson at bigfoot dot com

Hi,

Given I'm on gcc 4.8.1 (Debian 4.8.1-2). Given the following test program:

sesse@gruessi:~$ cat bextr-test.c 
#include <stdint.h>

uint64_t func(uint64_t x, uint64_t *y)
{
    return __builtin_ia32_bextr_u64(x, *y);
}

trying to compile it fails:

sesse@gruessi:~$ gcc-4.8 -O2 -mbmi -c bextr-test.c --save-temps
bextr-test.s: Assembler messages:
bextr-test.s:9: Error: operand size mismatch for `bextr'

seemingly because GCC's idea of r/m is broken for this instruction:


sesse@gruessi:~$ cat bextr-test.s
    .file    "bextr-test.c"
    .text
    .p2align 4,,15
    .globl    func
    .type    func, @function
func:
.LFB0:
    .cfi_startproc
    bextr    (%rsi), %rdi, %rax
    ret
    .cfi_endproc
.LFE0:
    .size    func, .-func
    .ident    "GCC: (Debian 4.8.1-2) 4.8.1"
    .section    .note.GNU-stack,"",@progbits

As far as I understand, the second operand can be r/m64, but the first can only
be r64.

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 30307
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30307&action=edit
gcc49-pr57623.patch

The fix for the compiler is easy, but at least the AVX2 spec documents that
_bextr_u{32,64} intrinsics actually take 3 arguments (source, start and
length),
with the latter two always unsigned int, while our intrinsic has only two
arguments (where the latter is expected to be (start & 255) | (length << 8)).
Not sure if we want to change this, and if so, just for 4.9+, or also for
4.8.2+ and 4.7.4+?

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[sgunderson at bigfoot dot com]

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

--- Comment #2 from sgunderson at bigfoot dot com ---
On Sat, Jun 15, 2013 at 04:33:14PM +0000, jakub at gcc dot gnu.org wrote:
> The fix for the compiler is easy, but at least the AVX2 spec documents that
> _bextr_u{32,64} intrinsics actually take 3 arguments (source, start and
> length),
> with the latter two always unsigned int, while our intrinsic has only two
> arguments (where the latter is expected to be (start & 255) | (length << 8)).
> Not sure if we want to change this, and if so, just for 4.9+, or also for
> 4.8.2+ and 4.7.4+?

If you decide to change it, at least consider keeping the old version around;
for instance, the start/length combination could come from a table. In
general, if you actually have to do shifting and stuff to create this
operand, the gain of the instruction is already lost.

/* Steinar */

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[sgunderson at bigfoot dot com]

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

--- Comment #4 from sgunderson at bigfoot dot com ---
On Sat, Jun 15, 2013 at 05:10:57PM +0000, jakub at gcc dot gnu.org wrote:
> If both start and length are constants, then it will be folded by the compiler,
> similarly if you use it inside of loop and start/length will be loop
> invariants, the computation can be hoisted out of the loop.

Sure, but again, neither of these match my situation. I really need to do a
lookup into a table (with a non-constant index) to get the value.

/* Steinar */

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Actually, seems the 3 argument Intel intrinsic is _bextr_u{32,64}, while the
GCC
intrinsic is __bextr_u{32,64}, so guess the two can coexist, we just need to
add the 3 argument one.

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[sgunderson at bigfoot dot com]

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

--- Comment #6 from sgunderson at bigfoot dot com ---
BZHI seems to have the same problem.

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 30387
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30387&action=edit
gcc49-pr57623.patch

BZHI has a similar problem, but due to the non-canonical order of AND arguments
in the pattern I doubt you can actually reproduce the problem (because combine
will never match it, so pretty much always will the insn use only register
arguments).  The attached patch should fix both of those issues.

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[sgunderson at bigfoot dot com]

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

--- Comment #8 from sgunderson at bigfoot dot com ---
I really did spot the BZHI problem in actual code; that's how I found out :-) I
rewrote it slightly and the problem disappeared, though.

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Then please provide preprocessed testcase for it (plus command line options). 
Because I'm really curious how it could have been matched.

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[sgunderson at bigfoot dot com]

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

--- Comment #12 from sgunderson at bigfoot dot com ---
Created attachment 30389
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30389&action=edit
BZHI bug example (compile with g++-4.8 -O2 -mbmi2 -c foo.cc)

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[sgunderson at bigfoot dot com]

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

--- Comment #13 from sgunderson at bigfoot dot com ---
There. That ought to satisfy your curiosity. :-) I get:

sesse@gruessi:~/addie$ g++-4.8 -O2 -mbmi2 -c foo.cc
/tmp/ccX2oEfE.s: Assembler messages:
/tmp/ccX2oEfE.s:21: Error: operand size mismatch for `bzhi'

due to

        bzhi    _ZL5shift(,%rax,8), %rdx, %rdx

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ah, right, it isn't the combiner in your case, but register allocator, yes,
that can happen.  The patch I've attached should fix that too.

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-06-27
     Ever confirmed|0                           |1

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Thu Jun 27 16:44:17 2013
New Revision: 200477

URL: http://gcc.gnu.org/viewcvs?rev=200477&root=gcc&view=rev
Log:
    PR target/57623
    * config/i386/i386.md (bmi2_bzhi_<mode>3): Swap AND arguments
    to match RTL canonicalization.  Swap predicates and
    constraints of operand 1 and 2.

    * gcc.target/i386/bmi2-bzhi-1.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/bmi2-bzhi-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
    trunk/gcc/testsuite/ChangeLog

Author: jakub
Date: Thu Jun 27 16:45:44 2013
New Revision: 200478

URL: http://gcc.gnu.org/viewcvs?rev=200478&root=gcc&view=rev
Log:
    PR target/57623
    * config/i386/i386.md (bmi2_bzhi_<mode>3): Swap AND arguments
    to match RTL canonicalization.  Swap predicates and
    constraints of operand 1 and 2.

    * gcc.target/i386/bmi2-bzhi-1.c: New test.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.target/i386/bmi2-bzhi-1.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/i386/i386.md
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

Author: jakub
Date: Thu Jun 27 16:48:05 2013
New Revision: 200479

URL: http://gcc.gnu.org/viewcvs?rev=200479&root=gcc&view=rev
Log:
    PR target/57623
    * config/i386/i386.md (bmi_bextr_<mode>): Swap predicates and
    constraints of operand 1 and 2.

    * gcc.target/i386/bmi-bextr-3.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/bmi-bextr-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
    trunk/gcc/testsuite/ChangeLog

Author: jakub
Date: Thu Jun 27 16:49:38 2013
New Revision: 200480

URL: http://gcc.gnu.org/viewcvs?rev=200480&root=gcc&view=rev
Log:
    PR target/57623
    * config/i386/i386.md (bmi_bextr_<mode>): Swap predicates and
    constraints of operand 1 and 2.

    * gcc.target/i386/bmi-bextr-3.c: New test.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.target/i386/bmi-bextr-3.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/i386/i386.md
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Wed May  7 15:54:34 2014
New Revision: 210166

URL: http://gcc.gnu.org/viewcvs?rev=210166&root=gcc&view=rev
Log:
    Backported from mainline
    2013-06-27  Jakub Jelinek  <jakub@redhat.com>

    PR target/57623
    * config/i386/i386.md (bmi2_bzhi_<mode>3): Swap AND arguments
    to match RTL canonicalization.  Swap predicates and
    constraints of operand 1 and 2.

    * gcc.target/i386/bmi2-bzhi-1.c: New test.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.target/i386/bmi2-bzhi-1.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/i386/i386.md
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Wed May  7 15:57:48 2014
New Revision: 210168

URL: http://gcc.gnu.org/viewcvs?rev=210168&root=gcc&view=rev
Log:
    Backported from mainline
    2013-06-27  Jakub Jelinek  <jakub@redhat.com>

    PR target/57623
    * config/i386/i386.md (bmi_bextr_<mode>): Swap predicates and
    constraints of operand 1 and 2.

    * gcc.target/i386/bmi-bextr-3.c: New test.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.target/i386/bmi-bextr-3.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/i386/i386.md
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog

[Bug target/57623] BEXTR intrinsic has memory operands switched around (fails to compile code)

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Should be fixed now.