Bug report: wrong sign injection when encountering NaNs

[flaviens]

Hi there!

I've detected a bug in VexRiscv.

Brief bug description

Some sign injection instructions, such as fsgnj.d or fsgnjx.d, behave incorrectly when encountering NaNs.

Example instance

Snippet

Here is an example RISC-V (rv32imfd) snippet:

  .section ".text.init","ax",@progbits
  .globl _start
  .align 2
_start:

  # Enable the FPU
  li t0, 0x2000
  csrs mstatus, t0
  csrw	fcsr,x0

  la t0, .fdata0
  la t1, .fdata1

  fld f14, 0(t0)
  fld f3, 0(t1)
  fsgnj.d	f4,f14,f3

  li t2, 0x80000018
  fsd f4, 0(t2)

  sw x0, (x0)

infinite_loop:
  jal	x0,infinite_loop

.section ".fdata0","ax",@progbits
  .8byte 0xffffffff7fc00000
.section ".fdata1","ax",@progbits
  .8byte 0x7ff8000000000000

Waves

Here is the VCD trace of the execution of this program.

Expected and actual results

We expect f4 = 0x7fffffff7fc00000 because the sign bit of f3 is 0. I verified this with Spike. Other designs such as Rocket and BOOM provide this result as well.
However, VexRiscv returns f4 = 0xffffffff7fc00000.

The specification says:

Floating-point to floating-point sign-injection instructions, FSGNJ.S, FSGNJN.S, and FSGNJX.S, produce a result that takes all bits except the sign bit from rs1. For FSGNJ, the result’s sign bit is rs2’s sign bit [...] Sign-injection instructions do not set floating-point exception flags, nor do they canonicalize NaN

Fix

I identified the area probably responsible for the sign injection, which is

VexRiscv/src/main/scala/vexriscv/ip/fpu/FpuCore.scala

Lines 797 to 857 in b29eb54

	val sgnjRs1Sign = CombInit(input.rs1.sign)
	val sgnjRs2Sign = CombInit(input.rs2.sign)
	if(p.withDouble){
	sgnjRs2Sign setWhen(input.rs2Boxed && input.format === FpuFormat.DOUBLE)
	}
	val sgnjResult = (sgnjRs1Sign && input.arg(1)) ^ sgnjRs2Sign ^ input.arg(0)
	val fclassResult = B(0, 32 bits)
	val decoded = input.rs1.decode()
	fclassResult(0) := input.rs1.sign && decoded.isInfinity
	fclassResult(1) := input.rs1.sign && isNormal
	fclassResult(2) := input.rs1.sign && isSubnormal
	fclassResult(3) := input.rs1.sign && decoded.isZero
	fclassResult(4) := !input.rs1.sign && decoded.isZero
	fclassResult(5) := !input.rs1.sign && isSubnormal
	fclassResult(6) := !input.rs1.sign && isNormal
	fclassResult(7) := !input.rs1.sign && decoded.isInfinity
	fclassResult(8) := decoded.isNan && !decoded.isQuiet
	fclassResult(9) := decoded.isNan && decoded.isQuiet
	switch(input.opcode){
	is(FpuOpcode.STORE) { result := recodedResult }
	is(FpuOpcode.FMV_X_W) { result := recodedResult }
	is(FpuOpcode.F2I) { result(31 downto 0) := f2i.result.asBits }
	is(FpuOpcode.CMP) { result(31 downto 0) := cmpResult.resized }
	is(FpuOpcode.FCLASS) { result(31 downto 0) := fclassResult.resized }
	}
	rfOutput.valid := input.valid && toFpuRf && !halt
	rfOutput.source := input.source
	rfOutput.rd := input.rd
	rfOutput.roundMode := input.roundMode
	if(p.withDouble) rfOutput.format := input.format
	rfOutput.scrap := False
	rfOutput.value.sign := input.rs1.sign
	rfOutput.value.exponent := input.rs1.exponent
	rfOutput.value.mantissa := input.rs1.mantissa @@ U"0"
	rfOutput.value.special := input.rs1.special
	switch(input.opcode){
	is(FpuOpcode.MIN_MAX){
	when(minMaxSelectRs2) {
	rfOutput.value.sign := input.rs2.sign
	rfOutput.value.exponent := input.rs2.exponent
	rfOutput.value.mantissa := input.rs2.mantissa @@ U"0"
	rfOutput.value.special := input.rs2.special
	}
	when(minMaxSelectNanQuiet){
	rfOutput.value.setNanQuiet
	}
	}
	is(FpuOpcode.SGNJ){
	when(!input.rs1.isNan) {
	rfOutput.value.sign := sgnjResult
	}
	if(p.withDouble) when(input.rs1Boxed && input.format === FpuFormat.DOUBLE){
	rfOutput.value.sign := input.rs1.sign
	rfOutput.format := FpuFormat.FLOAT
	}
	}

However, I am very unfamiliar with the design itself and with SpinalHDL.
If you provide some guidance, I would be happy to help.

Thanks!
Flavien

[Dolu1990]

Hi ^^

It is a known behaviour "documented" as "Very specific, but SGNJ instruction will not mutate the value from/to F32/F64 (no NaN-boxing mutation)" in the main readme.md :)

I will have to take a look, i'm not sure if this is a bug which could create practical issues, i would say it isn't ?

[flaviens]

Ah cool, I missed it! What would it cost to fix it?

[Dolu1990]

What would it cost to fix it?

Some headaches i think ^^
It was related to how the floatpoint was re-encoded internaly i think, it was a long time ago XD

On NaxRiscv (https://spinalhdl.github.io/NaxRiscv-Rtd/main/NaxRiscv/introduction/index.html) i got it right, mostly because there was no re-encoding

[flaviens]

I'm willing to help! 🙂 if you have some indications I can try.

[Dolu1990]

I think, the main issue was that mutating the sign, in the case we have a nan boxed f32, would transform it into a f64. Probably something in that kind.
The thing is, internaly the float are reencodded in a non-ieee format, so not something realy made for bit maniuplation.

Main things to care then is how the last stage handle this (rounding, reformating) :
https://github.com/SpinalHDL/VexRiscv/blob/fpu-fixes/src/main/scala/vexriscv/ip/fpu/FpuCore.scala#L1541

Here is how the internal used to store float value :
https://github.com/SpinalHDL/VexRiscv/blob/fpu-fixes/src/main/scala/vexriscv/ip/fpu/Interface.scala#L33

All sgnj op goes into the "shortPip".
See https://github.com/SpinalHDL/VexRiscv/blob/fpu-fixes/src/main/scala/vexriscv/ip/fpu/FpuCore.scala#L602

Would have to check what that pipeline get in all cases (nan, nan boxed, others) to see how to properly handle it.
Did it reaaaaly long ago, so don't remember too much about it.