Bug Fix: correcting immediate encoding for unsigned instructions

[GabPGomes]

Bug fix

First of all, congratulations for your work in the darkriscv core! It is a very popular processor. I'm working on a project that involves RISC-V processors verification, and I used darkriscv in my evaluation.
Our Project

The Issue

Darkriscv is failing to execute the SLTIU instruction when the immediate is negative. The darkriscv source code uses an "unsigned immediate" to perform the SLTIU comparison, which is the immediate bits extended with zeroes.

darkriscv/rtl/darkriscv.v

Lines 195 to 201 in 61bd172

	XUIMM <= HLT ? XUIMM :
	IDATAX[6:0]==`SCC ? { ALL0[31:12], IDATAX[31:25],IDATAX[11:7] } : // s-type
	IDATAX[6:0]==`BCC ? { ALL0[31:13], IDATAX[31],IDATAX[7],IDATAX[30:25],IDATAX[11:8],ALL0[0] } : // b-type
	IDATAX[6:0]==`JAL ? { ALL0[31:21], IDATAX[31], IDATAX[19:12], IDATAX[20], IDATAX[30:21], ALL0[0] } : // j-type
	IDATAX[6:0]==`LUI||
	IDATAX[6:0]==`AUIPC ? { IDATAX[31:12], ALL0[11:0] } : // u-type
	{ ALL0[31:12], IDATAX[31:20] }; // i-type

However, according to the RISC-V manual v20250508, page 25, all immediate must be sign-extended, even if later evaluated as unsigned.

The only instructions that use this kind of unsigned immediate are CSR instructions that I believe were not implemented in darkriscv. This was extracted from the same manual, page 49.

How to Reproduce

This code snippet from the riscv-arch-test suite is enough to reproduce the error:

800002c4 <inst_21>:
800002c4:	40000eb7          	lui	t4,0x40000
800002c8:	ff7eb893          	sltiu	a7,t4,-9
800002cc:	01112623          	sw	a7,12(sp)

The Fix

Since all immediates must be sign-extended, I removed the unsigned immediates.

[samsoniuk]

Wow! Thanks for the bug! =D

It is amazing that such bug was hidden for so many years but, in a quick check, I grepped 1M+ lines of RISCV asm code here and found not a single sltiu or slti instruction! That explain why the bug survived across the years.

Anyway, I need patch some cores by hand here and test in order to ensure everything is working but I will need some time...

Meanwhile, I would suggest redo the PR to keep the changes at the minimum:

$ git diff rtl/darkriscv.v
diff --git a/rtl/darkriscv.v b/rtl/darkriscv.v
index ce8b13e..8e5f105 100644
--- a/rtl/darkriscv.v
+++ b/rtl/darkriscv.v
@@ -481,7 +481,7 @@ module darkriscv
     // RM-group of instructions (OPCODEs==7'b0010011/7'b0110011), merged! src=immediate(M)/register(R)
 
     wire signed [31:0] S2REGX = XMCC ? SIMM : S2REG;
-    wire        [31:0] U2REGX = XMCC ? UIMM : U2REG;
+    wire        [31:0] U2REGX = XMCC ? SIMM : U2REG; // all imm must be sign-extended
 
     wire [31:0] RMDATA = FCT3==7 ? U1REG&S2REGX :
                          FCT3==6 ? U1REG|S2REGX :

Which means keep XSIMM/SIMM and XUIMM/UIMM naming, in a way that the merge with older versions is less complex. As far as XUIMM/UIMM are not used, they will be optimized out at RTL steps, while the same naming scheme keeps a more smooth path between past and future versions of the code.

About the Processor CI project, it is a very interesting project and I hope you find a time to fix the bugs for all that processors in the list! For sure, not an easy task, because each processor probably have very specific requirements in order to work! Of course, case you need some help, please make me know about it! =D

In the case of DarkRISCV, it was almost running in early versions but I made lots of changes in the top level in order to make it work with caches and asynchronous buses, so that changes probably broke the compatibility with your top level.

Early versions of DarkRISCV relied on external control to halt the processor in order to insert wait states, while the more recent versions added a new control flow per interface, similar to Wishbone bus, so data and code interfaces can now better control the wait-states when interfacing memories, so it will probably work in the future with a reviewed top level.

For now thanks for the bug and good hacking!
Marcelo

[GabPGomes]

Hi, Marcelo
Thank you for the feedback! I've updated the pull request as you suggested.

[samsoniuk]

Thanks! =D