[Bug target/116278] New: [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[Bug target/116278] New: [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[patrick at rivosinc dot com]

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

            Bug ID: 116278
           Summary: [15] RISC-V: Miscompile at -O2 -fwrapv
                    -fno-strict-aliasing
           Product: gcc
           Version: 15.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: patrick at rivosinc dot com
  Target Milestone: ---

Testcase:
signed char b[1];
int c;
signed char *d = b;
int main() {
  b[0] = -40;
  c = ({ (unsigned short)d[0] < 0xFFF6 ? (unsigned short)d[0] : 0xFFF6; }) + 9;
  __builtin_printf("%d\n", c);
}

Commands:
> /scratch/tc-testing/tc-compiler-fuzz-trunk/build-gcv/bin/riscv64-unknown-linux-gnu-gcc -O2 -fwrapv -fno-strict-aliasing red.c -o red.out
> /scratch/tc-testing/tc-compiler-fuzz-trunk/build-gcv/bin/qemu-riscv64 red.out
65535
> /scratch/tc-testing/tc-compiler-fuzz-trunk/build-gcv/bin/riscv64-unknown-linux-gnu-gcc -O1 -fwrapv -fno-strict-aliasing red.c -o red.out
> /scratch/tc-testing/tc-compiler-fuzz-trunk/build-gcv/bin/qemu-riscv64 red.out
65505

Found via fuzzer

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This might be a SAT_ADD issue.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[kito at gcc dot gnu.org]

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

Kito Cheng <kito at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kito at gcc dot gnu.org,
                   |                            |pan2.li at intel dot com

--- Comment #2 from Kito Cheng <kito at gcc dot gnu.org> ---
Hi Pan, could you take a look to see if it related to SAT_ADD?

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pan2.li at intel dot com]

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

--- Comment #3 from Li Pan <pan2.li at intel dot com> ---
(In reply to Kito Cheng from comment #2)
> Hi Pan, could you take a look to see if it related to SAT_ADD?

Ack, thanks.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
        lb      a1,0(a5) // load -40
        lui     a0,%hi(.LC0)
        lui     a4,%hi(c)
        addi    a5,a1,9 //a5 = -31
        slli    a5,a5,48 
        srli    a5,a5,48 // a5 = -31
        sltu    a1,a5,a1 // -31 <u -40 ? false
        neg     a1,a1 // 0
        or      a1,a1,a5 // -31
        slli    a1,a1,48
        srli    a1,a1,48

I don't see anything wrong here ... Unless I missed something (is lb a signed
load byte?)

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pan2.li at intel dot com]

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

--- Comment #5 from Li Pan <pan2.li at intel dot com> ---
Reproduced from both qemu and hardware, let me take a look.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pan2.li at intel dot com]

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

--- Comment #6 from Li Pan <pan2.li at intel dot com> ---
(In reply to Andrew Pinski from comment #4)
>         lb      a1,0(a5) // load -40
>         lui     a0,%hi(.LC0)
>         lui     a4,%hi(c)
>         addi    a5,a1,9 //a5 = -31
>         slli    a5,a5,48 
>         srli    a5,a5,48 // a5 = -31
>         sltu    a1,a5,a1 // -31 <u -40 ? false
>         neg     a1,a1 // 0
>         or      a1,a1,a5 // -31
>         slli    a1,a1,48
>         srli    a1,a1,48
> 
> I don't see anything wrong here ... Unless I missed something (is lb a
> signed load byte?)

I think the problem comes from the lb side

     0x1014c <main+16>               lb      a1,0(a5)  // a1 is -40, aka
0xffffffffffffffd8                                                             
                                                                               
         ││   0x10150 <main+20>               lui     a0,0x1a                  
                                                                               
                                                          ││   0x10152
<main+22>               addi    a5,a1,9                                        
                                                                               
                                    ││   0x10156 <main+26>               slli  
 a5,a5,0x30                                                                    
                                                                               
     ││   0x10158 <main+28>               srli    a5,a5,0x30 // a5 is 65505    
                                                                               
                                                                     ││ 
>0x1015a <main+30>               sltu    a1,a5,a1   // compare 65505 and
0xffffffffffffffd8 => TRUE, and then get max 65535.

It should be one backend code-gen issue, will take care of this.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pan2.li at intel dot com]

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

--- Comment #7 from Li Pan <pan2.li at intel dot com> ---
The backend take 
rtx xmode_x = gen_lowpart (Xmode, x);

For the incoming op of .SAT_ADD, thus I think we should take lbu instead of lb
according to the ISA.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pan2.li at intel dot com]

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

--- Comment #8 from Li Pan <pan2.li at intel dot com> ---
(In reply to Li Pan from comment #7)
> The backend take 
> rtx xmode_x = gen_lowpart (Xmode, x);
> 
> For the incoming op of .SAT_ADD, thus I think we should take lbu instead of
> lb according to the ISA.

During usadd expanding the mode is HI instead of QI, thus we need zero_extend
for non-Xmodes, will prepare the fix soon.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pan Li <panli@gcc.gnu.org>:

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

commit r15-2979-ge8f31f4f58f0fcf1716fc1d9ee003fbcdda600c3
Author: Pan Li <pan2.li@intel.com>
Date:   Fri Aug 9 10:26:32 2024 +0800

    RISC-V: Make sure high bits of usadd operands is clean for non-Xmode
[PR116278]

    For QI/HImode of .SAT_ADD,  the operands may be sign-extended and the
    high bits of Xmode may be all 1 which is not expected.  For example as
    below code.

    signed char b[1];
    unsigned short c;
    signed char *d = b;
    int main() {
      b[0] = -40;
      c = ({ (unsigned short)d[0] < 0xFFF6 ? (unsigned short)d[0] : 0xFFF6; })
+ 9;
      __builtin_printf("%d\n", c);
    }

    After expanding we have:

    ;; _6 = .SAT_ADD (_3, 9);
    (insn 8 7 9 (set (reg:DI 143)
            (high:DI (symbol_ref:DI ("d") [flags 0x86]  <var_decl d>)))
         (nil))
    (insn 9 8 10 (set (reg/f:DI 142)
            (mem/f/c:DI (lo_sum:DI (reg:DI 143)
                    (symbol_ref:DI ("d") [flags 0x86]  <var_decl d>)) [1 d+0 S8
A64]))
         (nil))
    (insn 10 9 11 (set (reg:HI 144 [ _3 ])
            (sign_extend:HI (mem:QI (reg/f:DI 142) [0 *d.0_1+0 S1 A8])))
"test.c":7:10 -1
         (nil))

    The convert from signed char to unsigned short will have sign_extend rtl
    as above.  And finally become the lb insn as below:

    lb      a1,0(a5)   // a1 is -40, aka 0xffffffffffffffd8
    lui     a0,0x1a
    addi    a5,a1,9
    slli    a5,a5,0x30
    srli    a5,a5,0x30 // a5 is 65505
    sltu    a1,a5,a1   // compare 65505 and 0xffffffffffffffd8 => TRUE

    The sltu try to compare 65505 and 0xffffffffffffffd8 here,  but we
    actually want to compare 65505 and 65496 (0xffd8).  Thus we need to
    clean up the high bits to ensure this.

    The below test suites are passed for this patch:
    * The rv64gcv fully regression test.

            PR target/116278

    gcc/ChangeLog:

            * config/riscv/riscv.cc (riscv_gen_zero_extend_rtx): Add new
            func impl to zero extend rtx.
            (riscv_expand_usadd): Leverage above func to cleanup operands 0
            and remove the special handing for SImode in RV64.

    gcc/testsuite/ChangeLog:

            * gcc.target/riscv/sat_u_add-11.c: Adjust asm check body.
            * gcc.target/riscv/sat_u_add-15.c: Ditto.
            * gcc.target/riscv/sat_u_add-19.c: Ditto.
            * gcc.target/riscv/sat_u_add-23.c: Ditto.
            * gcc.target/riscv/sat_u_add-3.c: Ditto.
            * gcc.target/riscv/sat_u_add-7.c: Ditto.
            * gcc.target/riscv/sat_u_add_imm-11.c: Ditto.
            * gcc.target/riscv/sat_u_add_imm-15.c: Ditto.
            * gcc.target/riscv/sat_u_add_imm-3.c: Ditto.
            * gcc.target/riscv/sat_u_add_imm-7.c: Ditto.
            * gcc.target/riscv/pr116278-run-1.c: New test.
            * gcc.target/riscv/pr116278-run-2.c: New test.

    Signed-off-by: Pan Li <pan2.li@intel.com>

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[sjames at gcc dot gnu.org]

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

Sam James <sjames at gcc dot gnu.org> changed:

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

--- Comment #10 from Sam James <sjames at gcc dot gnu.org> ---
I would suggest making pr116278-run-{1,2}.c generic torture tests (outside of
riscv) but having either a scan assembler directive just on riscv, or a
separate copy of the test in riscv/ for the assembly check.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[pan2.li at intel dot com]

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

--- Comment #11 from Li Pan <pan2.li at intel dot com> ---
Thanks for suggestion, will move run test to
gcc/testsuite/gcc.c-torture/execute and only leave asm check under riscv.

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[sjames at gcc dot gnu.org]

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

--- Comment #12 from Sam James <sjames at gcc dot gnu.org> ---
Thank you!

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pan Li <panli@gcc.gnu.org>:

https://gcc.gnu.org/g:3178786c88761e47b3cbe700a97a0de2b6e133cb

commit r15-3244-g3178786c88761e47b3cbe700a97a0de2b6e133cb
Author: Pan Li <pan2.li@intel.com>
Date:   Mon Aug 19 10:02:46 2024 +0800

    Test: Move pr116278 run test to dg/torture [NFC]

    Move the run test of pr116278 to dg/torture and leave the risc-v the
    asm check under risc-v part.

            PR target/116278

    gcc/testsuite/ChangeLog:

            * gcc.target/riscv/pr116278-run-1.c: Take compile instead of run.
            * gcc.target/riscv/pr116278-run-2.c: Ditto.
            * gcc.dg/torture/pr116278-run-1.c: New test.
            * gcc.dg/torture/pr116278-run-2.c: New test.

    Signed-off-by: Pan Li <pan2.li@intel.com>

[Bug target/116278] [15] RISC-V: Miscompile at -O2 -fwrapv -fno-strict-aliasing

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
                 CC|                            |law at gcc dot gnu.org
         Resolution|---                         |FIXED

--- Comment #14 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Should be fixed on the trunk now.