| Message ID | orjz856mx1.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | | expand |
On 3/31/25 1:05 PM, Alexandre Oliva wrote: > > [testsuite] [riscv] limit vwaddsub-1.c to rv64 > > The desired vw{add,sub}.wx instructions don't come up on rv32 for the > first two functions, we get v{add,sub}.vx instead. > > I suppose this is an oversight, and something about the test is meant > for rv64 only, but the fact that the instruction is spelled out in the > intrinsic name and a different instruction is generated suggests > something may be wrong after all. > > > I could use a pointer to specs for these intrinsics and instructions to > tell for sure, but a closer look from someone more familiar with them > would definitely be welcome. > > Tested on x86_64-linux-gnu native, and gcc-14 target riscv{64,32}-elf. > Ok to install? > > > for gcc/testsuite/ChangeLog > > * gcc.target/riscv/rvv/base/vwaddsub-1.c: Require rv64. I don't immediately see anything in this test or its history to indicate it's only supposed to work for rv64. So it'd be better dig a bit further into this one. jeff
On Mar 31, 2025, Jeff Law <jeffreyalaw@gmail.com> wrote: > On 3/31/25 1:05 PM, Alexandre Oliva wrote: >> The desired vw{add,sub}.wx instructions don't come up on rv32 for >> the >> first two functions, we get v{add,sub}.vx instead. >> I suppose this is an oversight, and something about the test is >> meant >> for rv64 only, but the fact that the instruction is spelled out in the >> intrinsic name and a different instruction is generated suggests >> something may be wrong after all. >> for gcc/testsuite/ChangeLog >> * gcc.target/riscv/rvv/base/vwaddsub-1.c: Require rv64. > I don't immediately see anything in this test or its history to > indicate it's only supposed to work for rv64. It's the 64-bit integral argument rs1. On rv64, it's in a single 64-bit register, that needs to be narrowed to 32 bits and then sign extended to 64 bits to fit the semantics of vwadd.vx. On rv32, the 64-bit argument is passed in two separate registers, the narrowing amounts to picking the first of the pair, and at that point it becomes vadd.vx. Likewise vwsub.vx->vsub.vx. So the test is indeed meant for rv64, but only because there's another, presumably better way to perform the requested operation on rv32.
On 3/31/25 7:03 PM, Alexandre Oliva wrote: > On Mar 31, 2025, Jeff Law <jeffreyalaw@gmail.com> wrote: >> I don't immediately see anything in this test or its history to >> indicate it's only supposed to work for rv64. > > It's the 64-bit integral argument rs1. Right, but ISTM we ought to be able to handle a vector of 64bit integral types, especially from the intrinsics interface. Probably the thing to do is see what the intrinsics docs say. Or maybe Kito knows offhand. > > On rv64, it's in a single 64-bit register, that needs to be narrowed to > 32 bits and then sign extended to 64 bits to fit the semantics of > vwadd.vx. Ah. A vx form. That's the key here. Indeed I don't think we can have a 64bit argument for the scalar argument in a .vx form insn. So objection cleared. OK for the trunk. jeff
On Tue, Apr 1, 2025 at 12:47 PM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > > On 3/31/25 7:03 PM, Alexandre Oliva wrote: > > On Mar 31, 2025, Jeff Law <jeffreyalaw@gmail.com> wrote: > >> I don't immediately see anything in this test or its history to > >> indicate it's only supposed to work for rv64. > > > > It's the 64-bit integral argument rs1. > Right, but ISTM we ought to be able to handle a vector of 64bit integral > types, especially from the intrinsics interface. > > Probably the thing to do is see what the intrinsics docs say. Or maybe > Kito knows offhand. Long story, fortunately we have some record on that: https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/9 https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/199 > > > > > > On rv64, it's in a single 64-bit register, that needs to be narrowed to > > 32 bits and then sign extended to 64 bits to fit the semantics of > > vwadd.vx. > Ah. A vx form. That's the key here. Indeed I don't think we can have > a 64bit argument for the scalar argument in a .vx form insn. > > So objection cleared. OK for the trunk. > jeff > >
On 4/1/25 8:03 AM, Kito Cheng wrote: > On Tue, Apr 1, 2025 at 12:47 PM Jeff Law <jeffreyalaw@gmail.com> wrote: >> >> >> >> On 3/31/25 7:03 PM, Alexandre Oliva wrote: >>> On Mar 31, 2025, Jeff Law <jeffreyalaw@gmail.com> wrote: >>>> I don't immediately see anything in this test or its history to >>>> indicate it's only supposed to work for rv64. >>> >>> It's the 64-bit integral argument rs1. >> Right, but ISTM we ought to be able to handle a vector of 64bit integral >> types, especially from the intrinsics interface. >> >> Probably the thing to do is see what the intrinsics docs say. Or maybe >> Kito knows offhand. > > Long story, fortunately we have some record on that: > > https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/9 > https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/199 Thanks. While they don't address this issue specifically, Craig's comments seem to indicate a desire to support the intrinsics in these scenarios and the compiler has to deal with the 64bit integer problems on rv32. I don't think it changes anything for Alex, we still need to disable the test for rv32 as the expected sequence just isn't right in that case. jeff
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/vwaddsub-1.c b/gcc/testsuite/gcc.target/riscv/rvv/base/vwaddsub-1.c index 6e027a555f377..84d3c4cb4c717 100644 --- a/gcc/testsuite/gcc.target/riscv/rvv/base/vwaddsub-1.c +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/vwaddsub-1.c @@ -1,4 +1,4 @@ -/* { dg-do compile { target { ! riscv_abi_e } } } */ +/* { dg-do compile { target { { ! riscv_abi_e } && rv64 } } } */ /* { dg-add-options riscv_v } */ /* { dg-additional-options "-std=gnu99 -O3 -fno-schedule-insns -fno-schedule-insns2" } */
[testsuite] [riscv] limit vwaddsub-1.c to rv64 The desired vw{add,sub}.wx instructions don't come up on rv32 for the first two functions, we get v{add,sub}.vx instead. I suppose this is an oversight, and something about the test is meant for rv64 only, but the fact that the instruction is spelled out in the intrinsic name and a different instruction is generated suggests something may be wrong after all. I could use a pointer to specs for these intrinsics and instructions to tell for sure, but a closer look from someone more familiar with them would definitely be welcome. Tested on x86_64-linux-gnu native, and gcc-14 target riscv{64,32}-elf. Ok to install? for gcc/testsuite/ChangeLog * gcc.target/riscv/rvv/base/vwaddsub-1.c: Require rv64. --- .../gcc.target/riscv/rvv/base/vwaddsub-1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)