Fixes `sendBits()`'s handling of signed numbers in `svsim`'s `simulation-driver` in #4593 #4599

AptInit · 2025-01-07T08:53:54Z

Although I don't understand the reasons for not using the conversions provided by stringstream, this PR fixes one of the edge cases as mentioned in #4593 :

If bitWidth % 8 == 0, and the bits have a value of -1<<(bitWidth-1), then the most significant bit will be stripped off by mistake.

Fixes #4593

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

Bugfix

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

linux-foundation-easycla · 2025-01-07T08:53:57Z

The committers listed above are authorized under a signed CLA.

✅ login: AptInit / name: Jason Wang (7b864f7, c506b90, ff79631, b92be13, add78df, 8841f48)
✅ login: jackkoenig / name: Jack Koenig (97e1e64)

jackkoenig · 2025-01-07T15:20:57Z

svsim/src/main/resources/simulation-driver.cpp

+    if (mutableBytes[byteCount - 1] != 0b10000000) {
+      mutableBytes[byteCount - 1] &= signBitMask - 1;
+    }


What happens if the width isn't a multiple of 8? Can we add tests for like 33 bits? And also can we add a test for 40 bits or something to make sure 32 isn't special as a power of 2?

Sorry just saw your comment in #4593 (comment) explaining that this matters for multiples of 8. I still think it would be good to test a non-multiple of 8 width (e.g. 33) and a non-power-of-2 multiple of 2, (e.g. 40).

Sure, I am still very new to the Chisel project, if we want to test other bitWidths, how should we prepare the SystemVerilog file since now we would like to have a parameterized SIntWire module? I mean I can't figure out how svsim/src/main/scala/Workspace.elaborate() works.

I think it's way too much work to have parameterized modules here. Can we just settle for two different bitWidths here @jackkoenig ?

I think it's way too much work to have parameterized modules here. Can we just settle for two different bitWidths here

Yes I think that makes sense!

Actually both scanHexBits() and sendBits() are buggy, I think we might as well replace the home-made conversions with something provided by the C++'s standard library in the future.

Actually both scanHexBits() and sendBits() are buggy, I think we might as well replace the home-made conversions with something provided by the C++'s standard library in the future.

Do you want to do that in this PR or in a separate PR? Separate is fine if this fixes issues now.

The test you wrote is a good one because it's using the lower-level APIs, but as you noted it's a lot less flexible. We could more easily write parameterized tests using the higher-level Simulator API that's built on top of SVSim, some example tests are here:

chisel/src/test/scala/chiselTests/simulator/SimulatorSpec.scala

Line 20 in 4c8efa9

class SimulatorSpec extends AnyFunSpec with Matchers {

Do you want to do that in this PR or in a separate PR? Separate is fine if this fixes issues now.

Unfortunately I don't think the current fix is nearly enough to fix sendBits().

Both scanHexBits() and sendBits() should work now @jackkoenig .

Rewrite tests

AptInit · 2025-01-08T02:46:49Z

The tl;dr is, both sendBits() and scanHexBits() are buggy in different ways when handling signed signals.

This PR should fix problems related to sendBits().
For workarounds of the unfixed bugs, use UInt instead of SInt if possible.

Beside the aforementioned stuff, svsim's use of DPI seems to be a bit strange to me. svBitVecVal is used for interfacing data regardless of the actual widths. But I am not experienced with DPI enough to draw a conclusion yet.

…ve number `-1<<(bitWidth-1)` with `bitWidth % 8 == 1` is supplied.

jackkoenig

This looks great, thank you!

…ance#4599)

jackkoenig · 2025-01-09T18:12:44Z

@Mergifyio backport 6.x

mergify · 2025-01-09T18:13:29Z

backport 6.x

✅ Backports have been created

Backport to branch 6.x not needed, change already in branch 6.x

jackkoenig · 2025-01-09T18:54:46Z

I think Github screwed up and merged this PR twice. The first commit has the changes d4d3e97, the 2nd doesn't 05d2622. I think Mergify is finding the 2nd and, because it has no changes, finding nothing to backport. What a weird issue.

(cherry picked from commit d4d3e97)

…4606) (cherry picked from commit d4d3e97) Co-authored-by: Jason Wang <[email protected]>

AptInit added 2 commits January 6, 2025 23:36

Strip irrelevant bits only when needed (chipsalliance#4593)

7b864f7

Added test for chipsalliance#4593

8841f48

jackkoenig reviewed Jan 7, 2025

View reviewed changes

jackkoenig added the Bugfix Fixes a bug, will be included in release notes label Jan 7, 2025

AptInit added 2 commits January 7, 2025 18:12

Merge branch 'main' into main

ff79631

Fix more edge cases for simulation-driver's sendBits()

add78df

Rewrite tests

simulation-driver's scanHexBits() handles the case where a negati…

b92be13

…ve number `-1<<(bitWidth-1)` with `bitWidth % 8 == 1` is supplied.

jackkoenig added this to the 6.x milestone Jan 8, 2025

make checkFormat happy

c506b90

jackkoenig approved these changes Jan 9, 2025

View reviewed changes

jackkoenig enabled auto-merge (squash) January 9, 2025 01:13

Merge branch 'main' into main

97e1e64

jackkoenig pushed a commit to AptInit/chisel that referenced this pull request Jan 9, 2025

Fixes sendBits()'s handling of signed numbers in svsim (chipsalli…

d4d3e97

…ance#4599)

jackkoenig merged commit 05d2622 into chipsalliance:main Jan 9, 2025
15 checks passed

mergify bot added the Backported This PR has been backported label Jan 9, 2025

jackkoenig pushed a commit that referenced this pull request Jan 9, 2025

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

aacf93c

(cherry picked from commit d4d3e97)

jackkoenig mentioned this pull request Jan 9, 2025

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

Merged

14 tasks

chiselbot pushed a commit that referenced this pull request Jan 9, 2025

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

d9c2f28

…4606) (cherry picked from commit d4d3e97) Co-authored-by: Jason Wang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes `sendBits()`'s handling of signed numbers in `svsim`'s `simulation-driver` in #4593 #4599

Fixes `sendBits()`'s handling of signed numbers in `svsim`'s `simulation-driver` in #4593 #4599

AptInit commented Jan 7, 2025 •

edited by jackkoenig

Loading

linux-foundation-easycla bot commented Jan 7, 2025 •

edited

Loading

jackkoenig Jan 7, 2025

jackkoenig Jan 7, 2025

AptInit Jan 7, 2025

AptInit Jan 7, 2025

jackkoenig Jan 7, 2025

AptInit Jan 7, 2025

jackkoenig Jan 8, 2025

AptInit Jan 8, 2025

AptInit Jan 8, 2025

AptInit commented Jan 8, 2025

jackkoenig left a comment

jackkoenig commented Jan 9, 2025

mergify bot commented Jan 9, 2025 •

edited

Loading

jackkoenig commented Jan 9, 2025

Fixes sendBits()'s handling of signed numbers in svsim's simulation-driver in #4593 #4599

Fixes sendBits()'s handling of signed numbers in svsim's simulation-driver in #4593 #4599

Conversation

AptInit commented Jan 7, 2025 • edited by jackkoenig Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

linux-foundation-easycla bot commented Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AptInit commented Jan 8, 2025

jackkoenig left a comment

Choose a reason for hiding this comment

jackkoenig commented Jan 9, 2025

mergify bot commented Jan 9, 2025 • edited Loading

✅ Backports have been created

jackkoenig commented Jan 9, 2025

Fixes `sendBits()`'s handling of signed numbers in `svsim`'s `simulation-driver` in #4593 #4599

Fixes `sendBits()`'s handling of signed numbers in `svsim`'s `simulation-driver` in #4593 #4599

AptInit commented Jan 7, 2025 •

edited by jackkoenig

Loading

linux-foundation-easycla bot commented Jan 7, 2025 •

edited

Loading

mergify bot commented Jan 9, 2025 •

edited

Loading