[Bug target/110790] New: [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[Bug target/110790] New: [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[slyfox at gcc dot gnu.org]

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

            Bug ID: 110790
           Summary: [14 Regression] gcc -m32 generates invalid bit test
                    code on gmp-6.2.1
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: slyfox at gcc dot gnu.org
  Target Milestone: ---

Initially observed test failures on gmp-6.2.1 when building with gcc
r14-2736-gd07504725973cc

Extracted reproducer:

// $ cat /tmp/a.c
typedef unsigned long int mp_limb_t;
typedef const mp_limb_t * mp_srcptr;

__attribute__((noipa))
int
refmpn_tstbit_bad (mp_srcptr ptr, unsigned long bit)
{
  return (((ptr)[(bit)/(32 - 0)] & (((mp_limb_t) 1L) << ((bit)%(32 - 0)))) !=
0);
}

__attribute__((noipa, optimize(0)))
int
refmpn_tstbit_good (mp_srcptr ptr, unsigned long bit)
{
  return (((ptr)[(bit)/(32 - 0)] & (((mp_limb_t) 1L) << ((bit)%(32 - 0)))) !=
0);
}

__attribute__((noipa))
int
refmpn_tstbit (mp_srcptr ptr, unsigned long bit)
{
  if (refmpn_tstbit_bad (ptr, bit) != refmpn_tstbit_good (ptr, bit)) {
      __builtin_trap();
  }
  return refmpn_tstbit_bad (ptr, bit);
}

int main(){
    unsigned long num[] = { 0x3801ff9f, 0x0, 0x0, 0x0 };
    refmpn_tstbit(num, 0);
}

$ gcc -m32 -fomit-frame-pointer /tmp/a.c -o /tmp/a -O2 && /tmp/a
Illegal instruction (core dumped)
$ gcc -m32 -fomit-frame-pointer /tmp/a.c -o /tmp/a && /tmp/a
# ok

It looks like refmpn_tstbit_bad() does something odd with bit shifts:

Dump of assembler code for function refmpn_tstbit_bad:
   0x08049200 <+0>:     mov    0x8(%esp),%eax ; bit
   0x08049204 <+4>:     mov    0x4(%esp),%edx ; ptr
   0x08049208 <+8>:     mov    %eax,%ecx
   0x0804920a <+10>:    shr    $0x5,%ecx          ; ecx = bit / 32 (limbs)
   0x0804920d <+13>:    mov    (%edx,%ecx,4),%edx ; edx = ptr[ecx]
   0x08049210 <+16>:    bt     %eax,%edx          ; might be ok if 'bit' does
not overflow?
   0x08049213 <+19>:    setae  %al
   0x08049216 <+22>:    movzbl %al,%eax
   0x08049219 <+25>:    ret

I can't see any problems with this code on the data we pass here.

$ gcc -v
Using built-in specs.
COLLECT_GCC=/<<NIX>>/xgcc-14.0.0/bin/gcc
COLLECT_LTO_WRAPPER=/<<NIX>>/xgcc-14.0.0/libexec/gcc/i686-unknown-linux-gnu/14.0.0/lto-wrapper
Target: i686-unknown-linux-gnu
Configured with: ../source/configure --prefix=/<<NIX>>/xgcc-14.0.0
--with-gmp-include=/<<NIX>>/gmp-6.2.1-dev/include
--with-gmp-lib=/<<NIX>>/gmp-6.2.1/lib
--with-mpfr-include=/<<NIX>>/mpfr-4.2.0-dev/include
--with-mpfr-lib=/<<NIX>>/mpfr-4.2.0/lib --with-mpc=/<<NIX>>/libmpc-1.3.1
--with-native-system-header-dir=/<<NIX>>/bootstrap-stage0-glibc-bootstrapFiles/include
--with-build-sysroot=/ --program-prefix= --disable-lto --disable-libstdcxx-pch
--without-included-gettext --with-system-zlib --enable-checking=release
--enable-static --enable-languages=c,c++ --disable-multilib --enable-plugin
--disable-libcc1 --with-isl=/<<NIX>>/isl-0.20 --disable-bootstrap
--with-arch=i686 --with-native-system-header-dir=/include
--with-build-sysroot=/<<NIX>>/bootstrap-stage0-glibc-bootstrapFiles
--build=i686-unknown-linux-gnu --host=i686-unknown-linux-gnu
--target=i686-unknown-linux-gnu
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 14.0.0 99999999 (experimental) (GCC)

refmpn_tstbit_bad() gets compile

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[slyfox at gcc dot gnu.org]

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

Sergei Trofimovich <slyfox at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |roger at nextmovesoftware dot com

--- Comment #1 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
I suspect it's a r14-2728-g59c38ddfe052a4 (the only change I see in recent
commits that have any relevance here). I did not try to revert locally.

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Roger Sayle <sayle@gcc.gnu.org>:

https://gcc.gnu.org/g:9f66753ef48f37729a88735ae1a2bf2d2558e69f

commit r14-2743-g9f66753ef48f37729a88735ae1a2bf2d2558e69f
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Mon Jul 24 12:34:23 2023 +0100

    [Committed] PR target/110787: Revert QImode offsets in {zero,sign}_extract.

    My recent patch to use QImode for bit offsets in ZERO_EXTRACTs and
    SIGN_EXTRACTs in the i386 backend shouldn't have resulted in any change
    behaviour, but as reported by Rainer it produces a bootstrap failure in
    gm2.  This reverts the problematic patch whilst we investigate the
    underlying cause.

    Committed as obvious.

    2023-07-23  Roger Sayle  <roger@nextmovesoftware.com>

    gcc/ChangeLog
            PR target/110787
            PR target/110790
            Revert patch.
            * config/i386/i386.md (extv<mode>): Use QImode for offsets.
            (extzv<mode>): Likewise.
            (insv<mode>): Likewise.
            (*testqi_ext_3): Likewise.
            (*btr<mode>_2): Likewise.
            (define_split): Likewise.
            (*btsq_imm): Likewise.
            (*btrq_imm): Likewise.
            (*btcq_imm): Likewise.
            (define_peephole2 x3): Likewise.
            (*bt<mode>): Likewise
            (*bt<mode>_mask): New define_insn_and_split.
            (*jcc_bt<mode>): Use QImode for offsets.
            (*jcc_bt<mode>_1): Delete obsolete pattern.
            (*jcc_bt<mode>_mask): Use QImode offsets.
            (*jcc_bt<mode>_mask_1): Likewise.
            (define_split): Likewise.
            (*bt<mode>_setcqi): Likewise.
            (*bt<mode>_setncqi): Likewise.
            (*bt<mode>_setnc<mode>): Likewise.
            (*bt<mode>_setncqi_2): Likewise.
            (*bt<mode>_setc<mode>_mask): New define_insn_and_split.
            (bmi2_bzhi_<mode>3): Use QImode offsets.
            (*bmi2_bzhi_<mode>3): Likewise.
            (*bmi2_bzhi_<mode>3_1): Likewise.
            (*bmi2_bzhi_<mode>3_1_ccz): Likewise.
            (@tbm_bextri_<mode>): Likewise.

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[slyfox at gcc dot gnu.org]

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

--- Comment #3 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
The change fixed `gmp` and `mpfr` test suites on `i686-linux` for me. And also
a `grep` testsuite on `x86_64-linux`. Thank you!

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
   Target Milestone|---                         |14.0
           Keywords|                            |wrong-code
             Target|                            |i?86-*-*
         Resolution|---                         |FIXED

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[roger at nextmovesoftware dot com]

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

--- Comment #5 from Roger Sayle <roger at nextmovesoftware dot com> ---
I'll add this testcase to the testsuite, when I apply a corrected version of my
QImode offset patch to mainline.  On the bright side, we'll be generating more
efficient code for gmp's refmpn_tstbit by using the x86's bt instruction (it
just needs to use setc not setnc in this case).

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Roger Sayle <sayle@gcc.gnu.org>:

https://gcc.gnu.org/g:e68a31549d9827030391d837951be96a5f95f291

commit r14-2866-ge68a31549d9827030391d837951be96a5f95f291
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Sat Jul 29 17:07:58 2023 +0100

    [Committed] Use QImode for offsets in zero_extract/sign_extract in i386.md

    As suggested by Uros, this patch changes the ZERO_EXTRACTs and
SIGN_EXTRACTs
    in i386.md to consistently use QImode for bit offsets (i.e. third and
fourth
    operands), matching the use of QImode for bit counts in shifts and rotates.

    This iteration also corrects the "ne:QI" vs "eq:QI" mistake in the previous
    version, which was responsible for PR 110787 and PR 110790 and so was
    rapidly reverted last weekend.  New test cases have been added to check
    the correct behaviour.

    2023-07-29  Roger Sayle  <roger@nextmovesoftware.com>

    gcc/ChangeLog
            PR target/110790
            * config/i386/i386.md (extv<mode>): Use QImode for offsets.
            (extzv<mode>): Likewise.
            (insv<mode>): Likewise.
            (*testqi_ext_3): Likewise.
            (*btr<mode>_2): Likewise.
            (define_split): Likewise.
            (*btsq_imm): Likewise.
            (*btrq_imm): Likewise.
            (*btcq_imm): Likewise.
            (define_peephole2 x3): Likewise.
            (*bt<mode>): Likewise
            (*bt<mode>_mask): New define_insn_and_split.
            (*jcc_bt<mode>): Use QImode for offsets.
            (*jcc_bt<mode>_1): Delete obsolete pattern.
            (*jcc_bt<mode>_mask): Use QImode offsets.
            (*jcc_bt<mode>_mask_1): Likewise.
            (define_split): Likewise.
            (*bt<mode>_setcqi): Likewise.
            (*bt<mode>_setncqi): Likewise.
            (*bt<mode>_setnc<mode>): Likewise.
            (*bt<mode>_setncqi_2): Likewise.
            (*bt<mode>_setc<mode>_mask): New define_insn_and_split.
            (bmi2_bzhi_<mode>3): Use QImode offsets.
            (*bmi2_bzhi_<mode>3): Likewise.
            (*bmi2_bzhi_<mode>3_1): Likewise.
            (*bmi2_bzhi_<mode>3_1_ccz): Likewise.
            (@tbm_bextri_<mode>): Likewise.

    gcc/testsuite/ChangeLog
            PR target/110790
            * gcc.target/i386/pr110790-1.c: New test case.
            * gcc.target/i386/pr110790-2.c: Likewise.

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[slyfox at gcc dot gnu.org]

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

--- Comment #7 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
The new change did not break `grep`, `gmp` and `mpfr` on x86_64-linux and
i686-linux. Success!

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[pinskia at gcc dot gnu.org]

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

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I need some code generation help for gcc.target/i386/pr110790-2.c, I have a
patch where we now generate:
```
        movq    (%rdi,%rax,8), %rax
        shrq    %cl, %rax
        andl    $1, %eax
```

instead of previously:
```
        movq    (%rdi,%rax,8), %rax
        btq     %rsi, %rax
        setc    %al
        movzbl  %al, %eax
```

I suspect the sequence that contains shrq/and is better but I am 100% sure. We
still get btq when used with a conditional too.

[Bug target/110790] [14 Regression] gcc -m32 generates invalid bit test code on gmp-6.2.1

[ubizjak at gmail dot com]

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

--- Comment #9 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Andrew Pinski from comment #8)
> I need some code generation help for gcc.target/i386/pr110790-2.c, I have a
> patch where we now generate:
> ```
>         movq    (%rdi,%rax,8), %rax
>         shrq    %cl, %rax
>         andl    $1, %eax
> ```
> 
> instead of previously:
> ```
>         movq    (%rdi,%rax,8), %rax
>         btq     %rsi, %rax
>         setc    %al
>         movzbl  %al, %eax
> ```
> 
> I suspect the sequence that contains shrq/and is better but I am 100% sure.
> We still get btq when used with a conditional too.

The new sequence is better. It does not create a partial reg write (setc needs
a clearing XOR in fron of CC-setting instruction).