Change the width of static shift right #3824

jackkoenig · 2024-02-15T23:58:18Z

This requires a recent change to CIRCT (llvm/circt#6698).

Note: The release notes were updated after merging #3841

Once firtool is released with the linked change, bumped in this repository, and this PR is merged, I think we should crack 7.0.0-M1 as it will serve as a useful milestone for dealing with this semantic change.

I also added a CLI option to emulate the old behavior: --legacy-shift-right-width. I'm not wedded to the name so if anyone has a better name for it, please suggest it.

Instructions on using this option will be in the release notes but we should probably also add a page to the website about migrating from Chisel 6 to Chisel 7 that calls this out.

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

API modification

Desired Merge Strategy

Rebase

Release Notes

A UInt shifted right by a static amount >= its width will now result
in a 0-bit UInt
An SInt shifted right by a static amount >= its width will now result
in a 1-bit SInt (the sign bit)

This is a change for SInts which Chisel would treat the output as a 0-bit SInt. However, FIRRTL implemented different behavior where both UInts and SInts would result in 1-bit values (which shifted right by an amount >= the width of the input).

Users can emulate the old behavior by providing CLI option --use-legacy-shift-right-width. Users are encouraged to generate Verilog with and without this option and diff it to ensure the width change does not affect the correctness of their design. Note that this option is purely for code migration and should not be used long term--it will eventually be removed.

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.

azidar · 2024-02-16T18:43:16Z

src/test/scala/chiselTests/ChiselSpec.scala

@@ -124,38 +126,50 @@ trait ChiselRunners extends Assertions {
    assert(!runTester(t, additionalVResources, annotations))
  }

-  def assertKnownWidth(expected: Int)(gen: => Data): Unit = {
+  def assertKnownWidth(expected: Int, args: Iterable[String] = Nil)(gen: => Data)(implicit pos: Position): Unit = {


Huh, so this is for scalatest to properly pass the source locator to the assert? Interesting.

Indeed, I added this because a test was failing and I wasn't sure which one.

azidar

LGTM!

azidar · 2024-02-16T18:43:47Z

core/src/main/scala/chisel3/Bits.scala

-    binop(sourceInfo, UInt(this.width.shiftRight(that)), ShiftRightOp, validateShiftAmount(that))
+
+  // Implement legacy [buggy] UInt shr behavior for both Chisel and FIRRTL
+  @nowarn("msg=method shiftRight in class Width is deprecated")


What does nowarn mean again?

I deprecated Width.shiftRight and it would trigger a warning here (and warnings are errors), so this @nowarn suppresses it. Obviously we are allowed to use our own deprecated APIs, but the API is public so I couldn't/shouldn't just delete it.

* A UInt shifted right by a static amount >= its width will now result in a 0-bit UInt * An SInt shifted right by a static amount >= its width will now result in a 1-bit SInt (the sign bit) This is a change for SInts which Chisel would treat the output as a 0-bit SInt. However, FIRRTL implemented different behavior where both UInts and SInts would result in 1-bit values (which shifted right by an amount >= the width of the input). This makes the width behavior consistent with the FIRRTL 4.0.0 spec.

New ChiselOption UseLegacyShiftRightWidthBehavior with CLI --legacy-shift-right-width will configure Chisel to emulate the old Chisel and FIRRTL behavior for the width of static shift right. This option is only intended for migrating old code and will eventually be removed.

seldridge

LGTM

Nits and suggestions for compacting some code and tests.

core/src/main/scala/chisel3/Bits.scala

seldridge · 2024-02-17T03:55:18Z

core/src/main/scala/chisel3/Width.scala

+  def unsignedShiftRight(that: Int): Width = this.op(this, (a, b) => 0.max(a - that))
+  def signedShiftRight(that:   Int): Width = this.op(this, (a, b) => 1.max(a - that))
+  def dynamicShiftLeft(that:   Width): Width =


Mega-nit: the three new methods could all be made final. This would make them inconsistent with other methods here, though. There's also very limited risk here because this thing is sealed.

I think the sealing is more than sufficient. I probably don't actually want them to be final because I have the ulterior motive of possibly introducing a "WidthChanged" private subclass of KnownWidth that could capture information about semantic changes in widths and warn when they are observable. I don't intend to implement it any time soon but there may be future changes were we would want to.

seldridge · 2024-02-17T04:11:58Z

src/test/scala/chiselTests/SIntOps.scala

+    assertKnownWidth(4, args) {
+      val in = IO(Input(SInt(8.W)))
+      in >> 4
+    }
+    assertKnownWidth(0, args) {
+      val in = IO(Input(SInt(8.W)))
+      in >> 8
+    }
+    assertKnownWidth(0, args) {
+      val in = IO(Input(SInt(8.W)))
+      in >> 16
+    }
+    assertKnownWidth(0, args) {
+      val in = IO(Input(SInt(0.W)))
+      in >> 8
+    }
+    assertKnownWidth(0, args) {
+      val in = IO(Input(SInt(0.W)))
+      in >> 0
+    }
+    assertInferredWidth(4, args) {
+      val in = IO(Input(SInt(8.W)))
+      val w = WireInit(SInt(), in)
+      w >> 4
+    }
+    assertInferredWidth(1, args) {
+      val in = IO(Input(SInt(8.W)))
+      val w = WireInit(SInt(), in)
+      w >> 8
+    }
+    assertInferredWidth(1, args) {
+      val in = IO(Input(SInt(8.W)))
+      val w = WireInit(SInt(), in)
+      w >> 16
+    }
+    assertInferredWidth(1, args) {
+      val in = IO(Input(SInt(0.W)))
+      val w = WireInit(SInt(), in)
+      w >> 8
+    }
+    assertInferredWidth(1, args) {
+      val in = IO(Input(SInt(0.W)))
+      val w = WireInit(SInt(), in)
+      w >> 0
+    }


There's a lot of duplication between this and the previous test. Consider if the patch could combine them in some way.

There is related commonality between these and the UInt tests.

seldridge · 2024-02-17T04:16:19Z

src/main/scala/chisel3/stage/ChiselOptions.scala

+  val chiselCircuit:         Option[Circuit] = None,
+  val sourceRoots:           Vector[File] = Vector.empty,
+  val warningFilters:        Vector[WarningFilter] = Vector.empty,
+  val legacyShiftRightWidth: Boolean = false) {


Nit: the naming of this may be better as useLegacyShiftRightWidth, i.e., something which is more obviously true/false.

jackkoenig added the API Modification label Feb 15, 2024

jackkoenig added this to the 7.0 milestone Feb 15, 2024

jackkoenig requested a review from seldridge February 15, 2024 23:58

jackkoenig force-pushed the jackkoenig/static-shift-right-width-change branch from 6517cd0 to bb1abcd Compare February 16, 2024 00:37

azidar reviewed Feb 16, 2024

View reviewed changes

azidar approved these changes Feb 16, 2024

View reviewed changes

jackkoenig added 2 commits February 16, 2024 17:01

jackkoenig force-pushed the jackkoenig/static-shift-right-width-change branch from bb1abcd to cdb3793 Compare February 17, 2024 01:01

jackkoenig enabled auto-merge February 17, 2024 01:04

jackkoenig merged commit 0a437d8 into main Feb 17, 2024
13 checks passed

jackkoenig deleted the jackkoenig/static-shift-right-width-change branch February 17, 2024 01:15

seldridge reviewed Feb 17, 2024

View reviewed changes

SpriteOvO mentioned this pull request Feb 17, 2024

Inconsistent behavior about zero-width operands #3735

Closed

jackkoenig mentioned this pull request Feb 20, 2024

Static shift right width change part 2 #3841

Merged

14 tasks

jackkoenig mentioned this pull request Jun 10, 2024

Inconsistent width for ChiselEnums cast to UInt #4159

Closed

This was referenced Jun 21, 2024

Fix width of ChiselEnum values in emitted FIRRTL #4200

Merged

asUInt packing inconsistent for Bundles with type parameters #4223

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the width of static shift right #3824

Change the width of static shift right #3824

jackkoenig commented Feb 15, 2024 •

edited

Loading

azidar Feb 16, 2024

jackkoenig Feb 16, 2024

azidar left a comment

azidar Feb 16, 2024

jackkoenig Feb 16, 2024

seldridge left a comment

seldridge Feb 17, 2024

jackkoenig Feb 20, 2024

seldridge Feb 17, 2024

seldridge Feb 17, 2024

Change the width of static shift right #3824

Change the width of static shift right #3824

Conversation

jackkoenig commented Feb 15, 2024 • edited Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azidar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge left a 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

jackkoenig commented Feb 15, 2024 •

edited

Loading