Fixes sendBits()'s handling of signed numbers in svsim (#4599)

AptInit · web-flow · commit d4d3e97429eb · 2025-01-09T01:48:30.000Z
diff --git a/svsim/src/main/resources/simulation-driver.cpp b/svsim/src/main/resources/simulation-driver.cpp
@@ -215,41 +215,51 @@ static void sendReady() { writeMessage(MESSAGE_READY, "ready"); }
 static void sendAck() { writeMessage(MESSAGE_ACK, "ack"); }
 
 // This method may modify the bytes in the buffer
+// Scala's BigInt.toString(16) follows the format of
+// `s"${if(value<0) "-" else ""}${value.abs.toString(16)}"`
 static void sendBits(uint8_t *mutableBytes, int bitCount, bool isSigned) {
   if (bitCount <= 0) {
     failWithError("Cannot send 0-bit value.");
   }
-  if (isSigned && bitCount <= 1) {
-    failWithError("Cannot send 1-bit signed value.");
-  }
   writeMessageStart(MESSAGE_BITS);
   writeMessageBody("%08X ", bitCount) int byteCount = (bitCount + 7) / 8;
   if (isSigned) {
     uint8_t signBitMask;
+    // A very dirty way to sign-extend the most significant byte
+    //  when the signed number is negative
+    uint8_t lastByteNegativePad;
     switch (bitCount % 8) {
     case 1:
       signBitMask = 0b00000001;
+      lastByteNegativePad = 0b11111110;
       break;
     case 2:
       signBitMask = 0b00000010;
+      lastByteNegativePad = 0b11111100;
       break;
     case 3:
       signBitMask = 0b00000100;
+      lastByteNegativePad = 0b11111000;
       break;
     case 4:
       signBitMask = 0b00001000;
+      lastByteNegativePad = 0b11110000;
       break;
     case 5:
       signBitMask = 0b00010000;
+      lastByteNegativePad = 0b11100000;
       break;
     case 6:
       signBitMask = 0b00100000;
+      lastByteNegativePad = 0b11000000;
       break;
     case 7:
       signBitMask = 0b01000000;
+      lastByteNegativePad = 0b10000000;
       break;
     case 0:
       signBitMask = 0b10000000;
+      lastByteNegativePad = 0b00000000;
       break;
     }
     if (mutableBytes[byteCount - 1] & signBitMask) {
@@ -258,13 +268,17 @@ static void sendBits(uint8_t *mutableBytes, int bitCount, bool isSigned) {
       int carry = 1;
       for (int i = 0; i < byteCount; i++) {
         int byte = mutableBytes[i];
+        // We need to sign-extend the last byte
+        if (i == byteCount - 1) {
+          byte = mutableBytes[i] | lastByteNegativePad;
+        }
         byte = (~byte & 0xFF) + carry;
         carry = byte >> 8;
         mutableBytes[i] = (uint8_t)byte;
       }
+      // Since we sign-extended the number,
+      //  two's compliment just works, no need to strip bits (#4593)
     }
-    // Strip irrelevant bits
-    mutableBytes[byteCount - 1] &= signBitMask - 1;
   }
   for (int i = byteCount - 1; i >= 0; i--) {
     writeMessageBody("%02X", mutableBytes[i]);
@@ -410,6 +424,9 @@ int scanHexByteReverse(const char **reverseScanCursor,
 }
 
 /**
+ * Scala's BigInt.toString(16) follows the format of
+ * `s"${if(value<0) "-" else ""}${value.abs.toString(16)}"`
+ *
  * Returned value must be manually freed.
  */
 static uint8_t *scanHexBits(const char **scanCursor, const char *scanEnd,
@@ -423,21 +440,18 @@ static uint8_t *scanHexBits(const char **scanCursor, const char *scanEnd,
   }
 
   bool isNegative;
-  int valueBitCount;
   if (**scanCursor == '-') {
     (*scanCursor)++;
     if (reverseScanCursor < *scanCursor) {
       failWithError("Unexpected end of negative value when %s.", description);
     }
     isNegative = true;
-    if (bitCount <= 1) {
-      failWithError("Cannot scan 1-bit-wide negative value when %s.",
+    if (bitCount < 1) {
+      failWithError("Cannot scan 0-bit-wide negative value when %s.",
                     description);
     }
-    valueBitCount = bitCount - 1;
   } else {
     isNegative = false;
-    valueBitCount = bitCount;
   }
 
   int byteCount = (bitCount + 7) / 8;
@@ -447,7 +461,7 @@ static uint8_t *scanHexBits(const char **scanCursor, const char *scanEnd,
   const char *firstCharacterOfValue = *scanCursor;
   int carry = 1; // Only used when `isNegative` is true
   int scannedByteCount = 0;
-  int valueByteCount = (valueBitCount + 7) / 8;
+  int valueByteCount = (bitCount + 7) / 8;
   while (scannedByteCount < valueByteCount) {
     int scannedByte = scanHexByteReverse(&reverseScanCursor,
                                          firstCharacterOfValue, description);
@@ -473,11 +487,10 @@ static uint8_t *scanHexBits(const char **scanCursor, const char *scanEnd,
   // A mask of the "inapplicable" bits in the high order byte, used to determine
   // if we received too many bits for the value we are trying to scan. This
   // value could be calculated with bitwise operations, but I find a table to be
-  // cleaner and easier to understand. We use `valueBitCount` instead of
-  // `bitCount` because the sign bit should be `1` for negative numbers along
-  // with all of the other leading bits.
+  // cleaner and easier to understand. There's no sign bit in Scala's `BigInt.toString(16)`,
+  // instead a minus sign will be present.
   uint8_t highOrderByteMask;
-  switch (valueBitCount % 8) {
+  switch (bitCount % 8) {
   case 1:
     highOrderByteMask = 0b11111110;
     break;
diff --git a/svsim/src/test/resources/SIntTest.sv b/svsim/src/test/resources/SIntTest.sv
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: Apache-2.0
+
+module SIntWire #(parameter WIDTH = 32)
+(
+  input [WIDTH-1:0] in,
+  output [WIDTH-1:0] out
+);
+  assign out = in;
+endmodule
+
+// We are testing low-level APIs, no support for parameterized modules
+module SIntTest
+(
+  input [7:0] in_8,
+  output [7:0] out_8,
+  input [30:0] in_31,
+  output [30:0] out_31,
+  input [31:0] in_32,
+  output [31:0] out_32,
+  input [32:0] in_33,
+  output [32:0] out_33,
+  output [7:0] out_const_8,
+  output [30:0] out_const_31,
+  output [31:0] out_const_32,
+  output [32:0] out_const_33
+);
+
+SIntWire #(8) wire_8(.in(in_8), .out(out_8));
+SIntWire #(31) wire_31(.in(in_31), .out(out_31));
+SIntWire #(32) wire_32(.in(in_32), .out(out_32));
+SIntWire #(33) wire_33(.in(in_33), .out(out_33));
+assign out_const_8 = 8'h80;
+assign out_const_31 = 31'b1000000000000000000000000000000;
+assign out_const_32 = 32'b10000000000000000000000000000000;
+assign out_const_33 = 33'b100000000000000000000000000000000;
+
+endmodule
diff --git a/svsim/src/test/scala/BackendSpec.scala b/svsim/src/test/scala/BackendSpec.scala
@@ -183,6 +183,95 @@ trait BackendSpec extends AnyFunSpec with Matchers {
         val traceReader = new BufferedReader(new FileReader(s"${simulation.workingDirectoryPath}/trace.vcd"))
         traceReader.lines().count() must be > 1L
       }
+
+      // Check sendBits()
+      it("communicates data to the Scala side correctly (#4593)") {
+        import Resources._
+        workspace.reset()
+        workspace.elaborateSIntTest()
+        workspace.generateAdditionalSources()
+        simulation = workspace.compile(
+          backend
+        )(
+          workingDirectoryTag = name,
+          commonSettings = CommonCompilationSettings(),
+          backendSpecificSettings = compilationSettings,
+          customSimulationWorkingDirectory = None,
+          verbose = false
+        )
+        simulation.run(
+          verbose = false,
+          executionScriptLimit = None
+        ) { controller =>
+          val bitWidths: Seq[Int] = List(8, 31, 32, 33)
+          val outConstPorts = bitWidths.map(b => controller.port(s"out_const_${b}"))
+
+          controller.setTraceEnabled(true)
+
+          for (idxBitWidth <- 0 until bitWidths.length) {
+            val bitWidth = bitWidths(idxBitWidth)
+            val outConst = outConstPorts(idxBitWidth)
+            val outConstVal = BigInt(-1) << (bitWidth - 1)
+            var isOutConstChecked: Boolean = false
+            outConst.check(isSigned = true) { value =>
+              isOutConstChecked = true
+              assert(value.asBigInt === outConstVal)
+            }
+            assert(isOutConstChecked === false)
+            controller.completeInFlightCommands()
+            assert(isOutConstChecked === true)
+          }
+        }
+      }
+
+      // Check both scanHexBits() and sendBits()
+      it("communicates data from and to the Scala side correctly") {
+        simulation.run(
+          verbose = false,
+          executionScriptLimit = None
+        ) { controller =>
+          val bitWidths: Seq[Int] = List(8, 31, 32, 33)
+          val inPorts = bitWidths.map(b => controller.port(s"in_${b}"))
+          val outPorts = bitWidths.map(b => controller.port(s"out_${b}"))
+
+          controller.setTraceEnabled(true)
+
+          // Some values near bounds
+          def boundValues(bitWidth: Int): Seq[BigInt] = {
+            val minVal = BigInt(-1) << (bitWidth - 1)
+            val maxVal = (BigInt(1) << (bitWidth - 1)) - 1
+            val deltaRange = maxVal.min(BigInt(257))
+            val valueNearZero = for { v <- -deltaRange to deltaRange } yield v
+            val valueNearMax = for { delta <- BigInt(0) to deltaRange } yield maxVal - delta
+            val valueNearMin = for { delta <- BigInt(0) to deltaRange } yield minVal + delta
+            valueNearMin ++ valueNearZero ++ valueNearMax
+          }
+
+          for (idxBitWidth <- 0 until bitWidths.length) {
+            val bitWidth = bitWidths(idxBitWidth)
+            val in = inPorts(idxBitWidth)
+            val out = outPorts(idxBitWidth)
+
+            val inValues = boundValues(bitWidth)
+            val outValues = inValues
+            for ((inVal, outVal) <- inValues.zip(outValues)) {
+              in.set(inVal)
+              in.check(isSigned = true) { value =>
+                assert(value.asBigInt === inVal)
+              }
+              var isOutChecked: Boolean = false
+              out.check(isSigned = true) { value =>
+                isOutChecked = true
+                assert(value.asBigInt === inVal)
+              }
+              assert(isOutChecked === false)
+
+              controller.completeInFlightCommands()
+              assert(isOutChecked === true)
+            }
+          }
+        }
+      }
     }
   }
 }
diff --git a/svsim/src/test/scala/Resources.scala b/svsim/src/test/scala/Resources.scala
@@ -46,5 +46,75 @@ object Resources {
         )
       )
     }
+    def elaborateSIntTest(): Unit = {
+      workspace.addPrimarySourceFromResource(getClass, "/SIntTest.sv")
+      workspace.elaborate(
+        ModuleInfo(
+          name = "SIntTest",
+          ports = Seq(
+            new ModuleInfo.Port(
+              name = "in_8",
+              isSettable = true,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "in_31",
+              isSettable = true,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "in_32",
+              isSettable = true,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "in_33",
+              isSettable = true,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_8",
+              isSettable = false,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_31",
+              isSettable = false,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_32",
+              isSettable = false,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_33",
+              isSettable = false,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_const_8",
+              isSettable = false,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_const_31",
+              isSettable = false,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_const_32",
+              isSettable = false,
+              isGettable = true
+            ),
+            new ModuleInfo.Port(
+              name = "out_const_33",
+              isSettable = false,
+              isGettable = true
+            )
+          )
+        )
+      )
+    }
   }
 }
