[Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1

[Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1

[zsojka at seznam dot cz]

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

            Bug ID: 101403
           Summary: [12 Regression] wrong code with __builtin_bswap16() at
                    -O1
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zsojka at seznam dot cz
  Target Milestone: ---
              Host: x86_64-pc-linux-gnu
            Target: x86_64-pc-linux-gnu

Created attachment 51129
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51129&action=edit
reduced testcase

$ x86_64-pc-linux-gnu-gcc -O testcase.c
$ ./a.out 
Aborted


$ x86_64-pc-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=/repo/gcc-trunk/binary-latest-amd64/bin/x86_64-pc-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/repo/gcc-trunk/binary-trunk-r12-2234-20210709194853-g1798cac7a8b-checking-yes-rtl-df-extra-nobootstrap-amd64/bin/../libexec/gcc/x86_64-pc-linux-gnu/12.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /repo/gcc-trunk//configure --enable-languages=c,c++
--enable-valgrind-annotations --disable-nls --enable-checking=yes,rtl,df,extra
--disable-bootstrap --with-cloog --with-ppl --with-isl
--build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu
--target=x86_64-pc-linux-gnu --with-ld=/usr/bin/x86_64-pc-linux-gnu-ld
--with-as=/usr/bin/x86_64-pc-linux-gnu-as --disable-libstdcxx-pch
--prefix=/repo/gcc-trunk//binary-trunk-r12-2234-20210709194853-g1798cac7a8b-checking-yes-rtl-df-extra-nobootstrap-amd64
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.0.0 20210710 (experimental) (GCC)

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |roger at nextmovesoftware dot com
   Target Milestone|---                         |12.0
          Component|target                      |tree-optimization

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Most likely introduced by r12-2137.

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hjl.tools at gmail dot com
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
            Summary|[12 Regression] wrong code  |[12 Regression] wrong code
                   |with __builtin_bswap16() at |with __builtin_bswap16() at
                   |-O1                         |-O1 by r12-2137
   Last reconfirmed|                            |2021-07-10

--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> ---
This is introduced by

commit 4c619132b3f14dc5e672a7f2f0e09cb784193559
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Thu Jul 8 11:46:14 2021 +0100

    PR tree-optimization/40210: Fold (bswap(X)>>C1)&C2 to (X>>C3)&C2 in
match.pd

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[roger at nextmovesoftware dot com]

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

Roger Sayle <roger at nextmovesoftware dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |roger at nextmovesoftware dot com

--- Comment #3 from Roger Sayle <roger at nextmovesoftware dot com> ---
Testing a fix. Sorry for the inconvenience.  An even more reduced test case is:
unsigned int foo (unsigned int a)
{
  unsigned int u;
  /* b == 0x8000 */
  unsigned short b = __builtin_bswap16 (a);
  /* result = 0x0008 */
  // This works: return b >> 12;
  // This doesn't:
  return b >> (u, 12);
}

int main (void)
{
  unsigned int x = foo (0x80);
  if (x != 0x0008)
    __builtin_abort ();
  return 0;
}
The change in signedness of the right shift (from logical to arithmetic)
triggers an oversight in my recent folding optimization.

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Isn't the testcase UB? It reads uninitialized u (though it doesn't use the
value for anything).

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[zsojka at seznam dot cz]

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

--- Comment #5 from Zdenek Sojka <zsojka at seznam dot cz> ---
(In reply to Jakub Jelinek from comment #4)
> Isn't the testcase UB? It reads uninitialized u (though it doesn't use the
> value for anything).

You are very right about that! The value is used, even though the use is
optimised out.
I am using -fsanitize=undefined when reducing testcases, but this one wasn't
caught. The only warning I see with -Wall -W is "warning: left-hand operand of
comma expression has no effect [-Wunused-value]". valgrind also doesn't
complain because, obviously, the uninitialised read is optimised out.

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[cvs-commit at gcc dot gnu.org]

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

--- 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:5f5fbb550af7d9d6cb56ae8f607fea0eccaa9295

commit r12-2238-g5f5fbb550af7d9d6cb56ae8f607fea0eccaa9295
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Mon Jul 12 08:24:27 2021 +0100

    PR tree-optimization/101403: Incorrect folding of ((T)bswap(x))>>C

    My sincere apologies for the breakage.  My recent patch to fold
    bswapN(x)>>C where the constant C was large enough that the result
    only contains bits from the low byte, and can therefore avoid
    the byte swap contains a minor logic error.  The pattern contains
    a convert? allowing an extension to occur between the bswap and
    the shift.  The logic is correct if there's no extension, or the
    extension has the same sign as the shift, but I'd mistakenly
    convinced myself that these couldn't have different signedness.

    (T)bswap16(x)>>12 is (T)((unsigned char)x>>4) or (T)((signed char)x>>4).
    The bug is that for zero-extensions to signed type T, we need to use
    the unsigned char variant [the signedness of the byte shift is not
    (always) the same as the signedness of T and the original shift].

    Then because I'm now paranoid, I've also added a clause to handle
    the hypothetical (but in practice impossible) sign-extension to an
    unsigned type T, which can implemented as (T)(x<<8)>>12.

    2021-07-12  Roger Sayle  <roger@nextmovesoftware.com>

    gcc/ChangeLog
            PR tree-optimization/101403
            * match.pd ((T)bswap(X)>>C): Correctly handle cases where
            signedness of the shift is not the same as the signedness of
            the type extension.

    gcc/testsuite/ChangeLog
            PR tree-optimization/101403
            * gcc.dg/pr101403.c: New test case.

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Another question is whether we shouldn't extend the short_shift stuff in
build_binary_op also to COMPOUND_EXPRs with INTEGER_CST on the ultimate rhs.
unsigned int f1 (unsigned short x) { return x >> 12; }
unsigned int f2 (unsigned short x) { static int u; return x >> (u, 12); }
The C++ FE already handles these the same, as it does:
          tree const_op1 = fold_for_warn (op1);
          if (TREE_CODE (const_op1) != INTEGER_CST)
            const_op1 = op1;
But it won't handle
int f ();
unsigned int f3 (unsigned short x) { return x >> (f (), 12); }
either.  The C FE doesn't try to simplify op1 in any way.
I guess in those cases we don't really care about any side-effects in the
expression, what we are looking for is just whether the value is known
constant.
For diagnostics, it would be nice if we handled x >> (f (), 37) like we handle
x >> 37 if it is an out of bounds shift.
For the optimization, maybe the short_shift stuff could be repeated in
match.pd.

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 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:0192c3eedbc7e6fe703abd8b321f400ddb02adf7

commit r12-2242-g0192c3eedbc7e6fe703abd8b321f400ddb02adf7
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Mon Jul 12 10:59:08 2021 +0100

    Tweak testcase for PR tree-optimization/101403.

    Initialize unused variable u in compound expression.  Committed as obvious.

    2021-07-12  Roger Sayle  <roger@nextmovesoftware.com>
                Jakub Jelinek  <jakub@redhat.com>

    gcc/testsuite/ChangeLog
            PR tree-optimization/101403
            * gcc.dg/pr101403.c: Avoid (unimportant) uninitialized variable.

[Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137

[roger at nextmovesoftware dot com]

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

Roger Sayle <roger at nextmovesoftware dot com> changed:

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

--- Comment #9 from Roger Sayle <roger at nextmovesoftware dot com> ---
Fixed on mainline.