[Bug c/108140] New: tzcnt gives different result in debug vs release

[Bug c/108140] New: tzcnt gives different result in debug vs release

[levo.delellis at gmail dot com]

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

            Bug ID: 108140
           Summary: tzcnt gives different result in debug vs release
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: levo.delellis at gmail dot com
  Target Milestone: ---

This might be more than one bug and I gotten the compiler to crash. Tested on
apple ventura with an M2 but it may happen on ARMv8 linux. This slightly
different test also fails with O2 on my mac https://godbolt.org/z/xv883jMb9

gcc docs says 0 might be undefined, I understand that

> Built-in Function: int __builtin_ctz (unsigned int x) 
>     Returns the number of trailing 0-bits in x, starting at the least significant bit position. If x is 0, the result is undefined. 

From my understanding armv8 doesn't have count trailing zero, it implements it
using rbits (reverse bits) and clz. clz says when you give it a 64bit register
it'll return 64 on 0. 
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/CLZ--Count-Leading-Zeros-

Now here's the problem. I would think __builtin_ctz would be those two
instructions. So I tried the below, compiled and ran using `gcc -Wall -Wextra
test.c && ./a.out` and saw that it worked as expected. However mistake was
stopping there. Using `gcc -O2 -Wall -Wextra test.c && ./a.out` gets 456
instead, no warnings or anything. Looking at the assembly it appears the check
has been optimized out and 456 is used. 

Looking at the "ARM C Language Extensions Architecture Specification" it
suggested including arm_acle.h. So I replaced the line below with the following
line and still got the incorrect result

    unsigned long long tz = __clz((unsigned long long)__rbit(input));

I'm not sure if this is another bug but this crashes with -O2
https://godbolt.org/z/xv883jMb9 it also doesn't give me the result I expected.
rbit appears to give me 32 no matter what I write. Doc says it should give 64
https://developer.arm.com/documentation/ddi0596/2020-12/SIMD-FP-Instructions/RBIT--vector---Reverse-Bit-order--vector--?lang=en

Anyway I would like to be warned against these problems somehow. Homebrew on
mac doesn't seem to have undefined behavior sanitizer (although I'm new to mac
and may have set it up incorrectly). The ubsan would be great to warn against
this. Alternative a flag such as -Wprefer-intrinsic could help, when either the
built ins don't match the CPU behavior.

From what I can tell __builtin_ctzll doesn't seem to return 32, it seems like
it does return 64, when you compare the variable the optimizer seems to think
it will never be greater than 32 which was a problem in my code because I was
using bits >= 60 so I can't simply do >= 32.


        #include<stdio.h>
        int main(int argc, char *argv[])
        {       
                unsigned long long input = argc-1;
                unsigned long long v = __builtin_ctzll(input);
                printf("%d %d\n", argc, v >= 64 ? 123 : 456);
        }

[Bug c/108140] tzcnt gives different result in debug vs release

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
If you call __builtin_ctzll with argument 0, it is undefined behavior, you
can't make any assumptions, regardless what instructions you assume will be
used or are actually used.
If you write your code as x ? __builtin_ctzll (x) : 64 or similar, GCC if it
uses an instruction that guarantees that result for 0 might actually fold it
back into just that instruction unconditionally.

[Bug c/108140] tzcnt gives different result in debug vs release

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And -fsanitize=undefined is able to sanitize those.

[Bug c/108140] tzcnt gives different result in debug vs release

[levo.delellis at gmail dot com]

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

--- Comment #3 from Levo DeLellis <levo.delellis at gmail dot com> ---
It's late and realized I meant to write rbitll but I can't seem to edit/correct
my post

Jakub ty I'll do just that.

Do you happen to know if clang uses the same sanitizers? I didn't seem to get
that on clang but it isn't the latest release.

[Bug middle-end/108140] ICE expanding __rbit

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org
           Keywords|                            |ice-on-valid-code
          Component|c                           |middle-end
             Target|                            |aarch64-*-*
            Summary|tzcnt gives different       |ICE expanding __rbit
                   |result in debug vs release  |

--- Comment #4 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
When comment #0 says "this crashes at -O2", it means ICE in expand for the
'__rbit' intrinsic on this testcase, which is reproducible on 12.2 and trunk:

#include<stdio.h>
#include<arm_acle.h>
int main(int argc, char *argv[])
{       
        unsigned long long input = argc-1;
        unsigned long long v = __clz(__rbit(input));
        printf("%d %d\n", argc, v >= 64 ? 123 : 456);
}

I've edited the bug title to reflect this.

[Bug middle-end/108140] ICE expanding __rbit

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3
                 CC|                            |ktkachov at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-12-16
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #5 from ktkachov at gcc dot gnu.org ---
Confirmed the ICE and I'm testing a patch to fix that, thanks for the report

[Bug target/108140] ICE expanding __rbit

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:98756bcbe27647f263f2b312d1d933d70cf56ba9

commit r13-4777-g98756bcbe27647f263f2b312d1d933d70cf56ba9
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Dec 19 11:16:47 2022 +0000

    aarch64: PR target/108140 Handle NULL target in data intrinsic expansion

    In this PR we ICE when expanding the __rbit builtin with a NULL target rtx.
    I *think* that only happens when the result is unused and hence maybe we
shouldn't be expanding
    any RTL at all, but the ICE here is easily fixed by deriving the mode from
the type of the expression
    rather than the target.

    This patch does that.
    Bootstrapped and tested on aarch64-none-linux-gnu.

    gcc/ChangeLog:

            PR target/108140
            * config/aarch64/aarch64-builtins.cc
            (aarch64_expand_builtin_data_intrinsic): Handle NULL target.

    gcc/testsuite/ChangeLog:

            PR target/108140
            * gcc.target/aarch64/acle/pr108140.c: New test.

[Bug target/108140] ICE expanding __rbit

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Kyrylo Tkachov
<ktkachov@gcc.gnu.org>:

https://gcc.gnu.org/g:849c3cf7b4189342b4a0df941afddf8327585570

commit r12-9037-g849c3cf7b4189342b4a0df941afddf8327585570
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Dec 19 11:16:47 2022 +0000

    aarch64: PR target/108140 Handle NULL target in data intrinsic expansion

    In this PR we ICE when expanding the __rbit builtin with a NULL target rtx.
    I *think* that only happens when the result is unused and hence maybe we
shouldn't be expanding
    any RTL at all, but the ICE here is easily fixed by deriving the mode from
the type of the expression
    rather than the target.

    This patch does that.
    Bootstrapped and tested on aarch64-none-linux-gnu.

    gcc/ChangeLog:

            PR target/108140
            * config/aarch64/aarch64-builtins.cc
            (aarch64_expand_builtin_data_intrinsic): Handle NULL target.

    gcc/testsuite/ChangeLog:

            PR target/108140
            * gcc.target/aarch64/acle/pr108140.c: New test.

    (cherry picked from commit 98756bcbe27647f263f2b312d1d933d70cf56ba9)

[Bug target/108140] ICE expanding __rbit

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.

[Bug target/108140] ICE expanding __rbit

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

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

--- Comment #9 from ktkachov at gcc dot gnu.org ---
This should have been fixed for 12.3.