[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?