Incorrect codegen for signed operations mixed with unsigned

[poemonsense]

Type of issue: Bug Report

Please provide the steps to reproduce the problem:

git clone -b chisel-SInt https://github.com/OpenXiangShan/chisel-playground.git
cd chisel-playground
make verilog
./build.sh build/GCD.v # Verilator required

What is the current behavior?

Considering the following Chisel code in playground/src/GCD.scala:

trait HasTestIO { this: Module =>
  val io = IO(new Bundle {
    val in0 = Input(UInt(16.W))
    val in1 = Input(UInt(16.W))
    val in2 = Input(UInt(16.W))
    val result = Output(UInt(16.W))
  })
  dontTouch(io)

  def calc: UInt = io.in0 + (io.in1.asSInt / io.in2.asSInt).asUInt
}

class Test1 extends Module with HasTestIO {
  val result = RegNext(calc)

  io.result := result
}


class Test2 extends Module with HasTestIO {
  val result = Reg(UInt(16.W))
  result := calc

  io.result := result
}

Modules Test1 and Test2 are expected to produce the same result. However, the above code will simulate as follows with tests.foreach(_.io.in0 := 0xFFFF.U), tests.foreach(_.io.in1 := 0xFFFF.U), and tests.foreach(_.io.in2 := 0x0002.U).

$ ./build.sh build/GCD.v
Building GCD...
Archive ar -rcs VGCD__ALL.a VGCD__ALL.o
Running ./obj_dir/VGCD...
result0: fffe
result1: ffff
[1] %Error: GCD.v:165: Assertion failed in TOP.GCD: Assertion failed
    at GCD.scala:41 assert(tests(0).io.result === tests(1).io.result)

%Error: build/GCD.v:165: Verilog $stop
Aborting...
Aborted (core dumped)

The reason is that the division is inlined in Test1, producing an expression with unsigned addition plus a signed division, i.e., {1'h0, io_in0} + $signed({io_in1[15], io_in1}) / $signed({io_in2[15], io_in2});. Unfortunately, according to https://github.com/verilator/verilator/issues/4814, this signed division will be converted to unsigned division when used as an unsigned operand for another unsigned operation. Though this conversion seems non-intuitive to everyone, it is the correct behavior according to the IEEE SV spec, as confirmed by the Verilator developer as well as my tests on VCS.

Test2 behaves as expected with automatic logic [16:0] _result_T_2 = $signed({io_in1[15], io_in1}) / $signed({io_in2[15], io_in2}); and result <= io_in0 + _result_T_2[15:0];. The signed division results in 0 (-1 / 2 = -0.5 = 0), and the addition produces 0xffff.

What is the expected behavior? result0 and result1 should be the same.

Please tell us about your environment:

Chisel 6.0.0-RC2 with firtool-1.62.0

Other Information

I'm sorry, but I'm not sure whether this is a Chisel bug or a CIRCT bug. Please correct me if I should raise the issue to CIRCT.

What is the use case for changing the behavior?

I think few people are using SInt in Chisel. But it should be correct.

[seldridge]

Nice find, and thanks for working with the Verilator folks on getting this tracked down.

I think few people are using SInt in Chisel. But it should be correct.

Yes, I expect low impact as SInt has very limited use. We'll get this fixed.

[poemonsense]

BTW, why does Chisel/CIRCT adds an extra sign bit when converting UInt to SInt? It seems without this extra sign bit, the conversion still holds.

[poemonsense]

@seldridge Hi, do you have a timeline when this bug will be fixed? Thanks

[seldridge]

Sorry, I forgot about this. I reduced this just now to: https://github.com/llvm/circt/issues/6961

@dtzSiFive identified that there was a very similar bug related to arithmetic shift right which was fixed by adding the EBForceResultSigned binary flag. This same approach should work for division here. See: https://github.com/llvm/circt/commit/31070d0e71f056bdf7bead11c239b4a6bfe1d4f3

I'll try to get a fix in this week. If you're so motivated to send a patch earlier, that is always appreciated.

[poemonsense]

Thank you very much on working this. Unluckily we still don't have enough human resources on understanding the circt. Personally I've tried my best to learn Chisel/CIRCT, but it seems the complexity of CIRCT makes me struggling. Hopefully next time I can submit a patch to CIRCT myself.

[dtzSiFive]

Fixed with https://github.com/llvm/circt/pull/6966 , tested the GCD example fails the assertion before and does not after.

This should work when bumping Chisel to use the next CIRCT release including that change.

[poemonsense]

Fixed with llvm/circt#6966 , tested the GCD example fails the assertion before and does not after.

This should work when bumping Chisel to use the next CIRCT release including that change.

Thanks very much for all of you. We're continuously tracking the latest Chisel.

I'm closing this issue now. Please feel free to open this again if there're still issues to be cleared.

[seldridge]

For completeness, the output of the erroneous example will now produce:

// Generated by CIRCT firtool-1.73.0-56-g3ef492c80-dirty
module Test1(
  input         clock,
                reset,
  input  [15:0] io_in0,
                io_in1,
                io_in2,
  output [15:0] io_result
);

  reg [16:0] result;
  always @(posedge clock)
    result <= {1'h0, io_in0} + $signed($signed({io_in1[15], io_in1}) / $signed({io_in2[15], io_in2}));
  assign io_result = result[15:0];
endmodule