[Bug tree-optimization/109989] New: RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[Bug tree-optimization/109989] New: RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[joseph.faulls at imgtec dot com]

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

            Bug ID: 109989
           Summary: RISC-V: Missing sign extension with int to float
                    conversion with 64bit soft floats
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: joseph.faulls at imgtec dot com
  Target Milestone: ---

Created attachment 55166
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55166&action=edit
Preprocessed test

Hi,

This bug was discovered when running test
gcc.target/riscv/promote-type-for-libcall.c on O1 for march rv64imac.

There are a few moving parts to this, and I haven't been able to track down
where the bug lies due to not being at all familiar with gcc. But I've managed
to reduce the test to the following criteria:

Compilation flags:
-march=rv64imac -mabi=lp64 -O1 -ftree-slp-vectorize -funroll-loops

march can be any 64bit without f/d extension.
Removal of any of the other flags (with the given test case) will not cause the
bug.

I have confirmed the only difference between 12.1 and 13.1 is the missing sign
extension before the call to __floatsisf

Inlining the test file here for added comments:

#include <stdio.h>
#include <stdlib.h>
volatile float f[2];

int x[2] ;

int main() {
  int i;
  x[0] = -1;
  x[1] = 2; // Removal of this line avoids the bug

  for (i=0;i<1;++i){
    f[i] = x[i]; // Any attempt to printf x[i] here avoids the bug
  }

  if (f[0] != -1.0f) {
    abort();
  }
  return 0;
}

Thanks!

[Bug target/109989] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[maxim.blinov at imgtec dot com]

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

Maxim Blinov <maxim.blinov at imgtec dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |maxim.blinov at imgtec dot com

--- Comment #1 from Maxim Blinov <maxim.blinov at imgtec dot com> ---
Hi all,

I bisected the failure to commit

3496ca4e656, "RISC-V: Add runtime invariant support".

GCC seems to treat the x array (`x[2]`) as a 2 integer vector, and loads it
into a DI mode register. However after the patch it neglects to honour the fact
that we only really want the lower 32 bits (which corresponds to `x[0]`) when
passing it as an argument to `__floatsisf`.

Before the commit, we generate

(insn 12 11 13 2 (set (reg:DI 10 a0)
        (sign_extend:DI (subreg:SI (reg:DI 77) 0))) "../a.c":16:10 116
{extendsidi2}
     (expr_list:REG_DEAD (reg:DI 77)
        (nil)))

After the commit, we generate

(insn 12 11 13 2 (set (reg:SI 10 a0)
        (subreg:SI (reg:DI 77) 0)) "../a.c":16:10 178 {*movsi_internal}
     (expr_list:REG_DEAD (reg:DI 77)
        (nil)))

which consequently gets removed as a useless instruction during the lra pass.

[Bug target/109989] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[maxim.blinov at imgtec dot com]

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

--- Comment #2 from Maxim Blinov <maxim.blinov at imgtec dot com> ---
Just wanted to update that unfortunately I haven't had much time to look
further into this issue: if anyone else has any ideas, please feel free to have
a look at it.

[Bug target/109989] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[palmer at gcc dot gnu.org]

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

palmer at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-06-21
     Ever confirmed|0                           |1
                 CC|                            |palmer at gcc dot gnu.org

--- Comment #3 from palmer at gcc dot gnu.org ---
I can confirm this is a bug.  The shortest reproducer I can get is:

$ cat test.c
volatile float f[2];
int x[2];

float fconv(int x);

void func() {
  x[0] = -1;
  x[1] = 2; // Removal of this line avoids the bug

  for (int i = 0; i < 1; ++i)
    f[i] = x[i];
}
$ ./toolchain/install/bin/riscv64-unknown-linux-gnu-gcc test.c -S -o-
-march=rv64imac -mabi=lp64 -O1 -ftree-slp-vectorize -funroll-loops
-fdump-rtl-all
...
func:
        addi    sp,sp,-16
        sd      ra,8(sp)
        li      a0,3
        slli    ra,a0,32
        addi    a0,ra,-1
        lui     a5,%hi(x)
        sd      a0,%lo(x)(a5)
        call    __floatsisf
        lui     t0,%hi(f)
        sw      a0,%lo(f)(t0)
        ld      ra,8(sp)
        addi    sp,sp,16
        jr      ra
...

With the bug being that a0 contains a non-ABI-canonical value at the call to
__floatsisf.

[Bug target/109989] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[palmer at gcc dot gnu.org]

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

--- Comment #4 from palmer at gcc dot gnu.org ---
I left some cruft in that reproducer, it should have been

volatile float f[2];
int x[2];

void func() {
  x[0] = -1;
  x[1] = 2;

  for (int i = 0; i < 1; ++i)
    f[i] = x[i];
}

Not sure what's going on yet, but nothing jumps out in that bisected patch. 
I'm guessing we've just got something wrong with poly/scalar conversion,
there's a bunch of implicit assumptions based around si/di conversions in our
backend and I bet we're violating something there.  That's all been a sticking
point for a while and I think some of the Ventana guys were looking into
cleaning it up, maybe Jeff has a better idea?

[Bug target/109989] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[orehovpasha at gmail dot com]

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

Pash Osh <orehovpasha at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |orehovpasha at gmail dot com

--- Comment #5 from Pash Osh <orehovpasha at gmail dot com> ---
As my investigations shows, it is based on passing wasted a0 to
__floatsisf(SItype).
Gcc13 (AKA after given patch) do not cleanup a0.hi via sext.w to honor generic
calling convention. While libgcc's compiler do not honor SImode in arg of
__floatsisf(SItype) so it uses wasted a0.hi. Someplaces.

FYI see a diff of call (-fdump-rtl-combine):
     insn_cost 4 for    21: r81:DI=sign_extend(r88:SI)
    -insn_cost 4 for    22: a0:DI=r81:DI //gcc12 (AKA before patch), gcc14,
non-buildin func(SItype)
    +insn_cost 4 for    22: a0:SI=r81:DI#0 // gcc13 (AKA after patch)
     insn_cost 36 for    23: a0:SF=call [`__floatsisf'] argc:0


But gcc12 (AKA before given patch), gcc14(with -fdisable-tree-dom2 to mitigate
const propagation), or manual func(SItype) call like

    SFtype fsi (SItype  i);
    ...
    for (i=0;i<1;++i){
        //f[i]=x[i];
        f[i] = fsi(x[i]);
    }

always issues insn a0:DI=r81:DI and generates correct sext.w


So it is like call(expand) of buildin __floatsisf is broken in gcc13 till head!

## __floatsisf(SItype) analysis

Short: it ignores sometimes SItype attr and use bit63, which on
gcc13-buildin-call is wasted
    #define _FP_FROM_INT(fs, wc, X, rr, rsize, rtype){                  \
          if (r){                                                              
\
          rtype _FP_FROM_INT_ur = (r);                                  \
          if ((X##_s = ((r) < 0)))                                      \
            _FP_FROM_INT_ur = -_FP_FROM_INT_ur;                         \

      -fdump-rtl-combine:
    insn_cost 4 for   256: r226:DI=a0:DI //a0 contains SItype
    insn_cost 4 for     2: r169:DI=r226:DI
    insn_cost 4 for    23: pc={(r169:DI==0)?L238:pc}

    insn_cost 0 for   240: debug D#1 => sign_extend(r169:DI#0) 
    insn_cost 4 for    26: r154:DI=sign_extend(r169:DI#0)
    insn_cost 0 for    27: debug _FP_FROM_INT_ur => D#1#0

    insn_cost 4 for    31: r171:SI=r169:DI#0 0>>0x1f
    insn_cost 4 for    32: r134:DI=zero_extend(r171:SI#0)
    insn_cost 0 for    33: debug A_s => zero_extend(r134:DI#0)

    insn_cost 4 for    37: pc={(r169:DI>=0)?L43:pc}


      riscv64-rise.13.1$ ...objdump -S  ...libgcc_s.so.1
    000000000000fa34 <__floatsisf>:
    fa38: c53d      beqz    a0,faa6 //if(r), do not honor SI mode, and use bit
63
    fa52: 01f5549b  srliw   s1,a0,0x1f //X##_s=((r)<0)); Sign=bit31; Honors SI
mode
    fa56: 04054b63  bltz    a0,faac //if(r<0), do not honor SI mode, X##_s, use
bit 63


## Question: that does `SItype` for func arg means exactly? Does it means a0.hi
is undefined and do not use it in caller and into callee?

One more argument: SI_BITS==32 ! (#define SI_BITS (__CHAR_BIT__ * (int)
sizeof(SItype)))

## Question: Is it ok, codegen differs for buildin and for explicit func with
same signature?

## What is best way to fix:?
0. What is your word about  "SI mode VS calling convention (a0.sign_extend)"?

1. call buildin `__floatsisf((long long) x)` to drop shrink  `a0:SI=r143:DI#0`
near ./gcc/optabs.cc: expand_float (rtx, rtx ...) ?

2. fix softfloat `__floatsisf`(SI i){if(i){if(i<0)…) to override calling
convention and honor SI, do not use bit63 ?

3. backport from gcc14?

[Bug target/109989] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[law at gcc dot gnu.org]

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

--- Comment #6 from Jeffrey A. Law <law at gcc dot gnu.org> ---
floatsisf is going to be called through the libcall interface which has
different paths than normal function calls and I don't think the usual type
promotion rules apply to libcalls.    The details escape me, but we've run into
similar kinds of problems with narrow arguments on libcalls for other targets.

This is almost certainly a bad interaction between the risc-v backend and the
libcall code.

[Bug target/109989] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[orehovpasha at gmail dot com]

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

--- Comment #7 from Pash Osh <orehovpasha at gmail dot com> ---
Fix is found in gcc14:
git cherry-pick 7560f2b4e387ef43ef45ee9fb06efbad6ca0fedf

Author: Vineet Gupta <vineetg@rivosinc.com>
Date:   Wed Nov 1 14:46:33 2023 -0700
    RISC-V: fix TARGET_PROMOTE_FUNCTION_MODE hook for libcalls
    Fixes: 3496ca4e6566 ("RISC-V: Add runtime invariant support")

How to commit it to all gcc-13.1,... releases/gcc-13 ?
Where manual? What about cheery-pick?

[Bug target/109989] [11/12/13 only] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.4
            Summary|RISC-V: Missing sign        |[11/12/13 only] RISC-V:
                   |extension with int to float |Missing sign extension with
                   |conversion with 64bit soft  |int to float conversion
                   |floats                      |with 64bit soft floats

[Bug target/109989] [13 Regression] RISC-V: Missing sign extension with int to float conversion with 64bit soft floats caused by r13-2105

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |12.1.0, 14.1.0
      Known to fail|                            |13.1.0
            Summary|[13 Regression] RISC-V:     |[13 Regression] RISC-V:
                   |Missing sign extension with |Missing sign extension with
                   |int to float conversion     |int to float conversion
                   |with 64bit soft floats      |with 64bit soft floats
                   |                            |caused by r13-2105

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Was fixed for GCC 14 by r14-5060-g7560f2b4e387ef .