Skip to content

Commit

Permalink
Fix width of ChiselEnum values in emitted FIRRTL
Browse files Browse the repository at this point in the history
Temporarily preserve the old behavior under CLI option
--use-legacy-width (formerly known as --use-legacy-shift-right-width).
Users are encouraged to build Verilog with and without this option
enabled and diff the result to verify that this change in width behavior
did not silently affect the correctness of their designs.
  • Loading branch information
jackkoenig committed Jun 21, 2024
1 parent 00c7a62 commit b871e69
Show file tree
Hide file tree
Showing 13 changed files with 92 additions and 51 deletions.
4 changes: 2 additions & 2 deletions core/src/main/scala/chisel3/Bits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ sealed class UInt private[chisel3] (width: Width) extends Bits(width) with Num[U
}

override def do_>>(that: Int)(implicit sourceInfo: SourceInfo): UInt = {
if (Builder.legacyShiftRightWidth) legacyShiftRight(that)
if (Builder.useLegacyWidth) legacyShiftRight(that)
else binop(sourceInfo, UInt(this.width.unsignedShiftRight(that)), ShiftRightOp, validateShiftAmount(that))
}
override def do_>>(that: BigInt)(implicit sourceInfo: SourceInfo): UInt =
Expand Down Expand Up @@ -1045,7 +1045,7 @@ sealed class SInt private[chisel3] (width: Width) extends Bits(width) with Num[S
override def do_>>(that: Int)(implicit sourceInfo: SourceInfo): SInt = {
// We don't need to pad to emulate old behavior for SInt, just emulate old Chisel behavior with reported width.
// FIRRTL will give a minimum of 1 bit for SInt.
val newWidth = if (Builder.legacyShiftRightWidth) this.width.shiftRight(that) else this.width.signedShiftRight(that)
val newWidth = if (Builder.useLegacyWidth) this.width.shiftRight(that) else this.width.signedShiftRight(that)
binop(sourceInfo, SInt(newWidth), ShiftRightOp, validateShiftAmount(that))
}
override def do_>>(that: BigInt)(implicit sourceInfo: SourceInfo): SInt =
Expand Down
12 changes: 10 additions & 2 deletions core/src/main/scala/chisel3/ChiselEnum.scala
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,16 @@ abstract class EnumType(private[chisel3] val factory: ChiselEnum, selfAnnotating
def do_>=(that: EnumType)(implicit sourceInfo: SourceInfo): Bool =
compop(sourceInfo, GreaterEqOp, that)

override private[chisel3] def _asUIntImpl(first: Boolean)(implicit sourceInfo: SourceInfo): UInt =
pushOp(DefPrim(sourceInfo, UInt(width), AsUIntOp, ref))
override private[chisel3] def _asUIntImpl(first: Boolean)(implicit sourceInfo: SourceInfo): UInt = {
this.litOption match {
// This fixes an old bug that changes widths and thus silently changes behavior.
// See https://github.com/chipsalliance/chisel/issues/4159.
case Some(value) if !Builder.useLegacyWidth =>
value.U(width)
case _ =>
pushOp(DefPrim(sourceInfo, UInt(width), AsUIntOp, ref))
}
}

protected[chisel3] override def width: Width = factory.width

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ object Definition extends SourceInfoDoc {
new DynamicContext(
Nil,
context.throwOnFirstError,
context.legacyShiftRightWidth,
context.useLegacyWidth,
context.warningFilters,
context.sourceRoots,
Some(context.globalNamespace),
Expand Down
14 changes: 7 additions & 7 deletions core/src/main/scala/chisel3/internal/Builder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -472,12 +472,12 @@ private[chisel3] class ChiselContext() {
}

private[chisel3] class DynamicContext(
val annotationSeq: AnnotationSeq,
val throwOnFirstError: Boolean,
val legacyShiftRightWidth: Boolean,
val warningFilters: Seq[WarningFilter],
val sourceRoots: Seq[File],
val defaultNamespace: Option[Namespace],
val annotationSeq: AnnotationSeq,
val throwOnFirstError: Boolean,
val useLegacyWidth: Boolean,
val warningFilters: Seq[WarningFilter],
val sourceRoots: Seq[File],
val defaultNamespace: Option[Namespace],
// Definitions from other scopes in the same elaboration, use allDefinitions below
val loggerOptions: LoggerOptions,
val definitions: ArrayBuffer[Definition[_]],
Expand Down Expand Up @@ -949,7 +949,7 @@ private[chisel3] object Builder extends LazyLogging {
major.toInt
}

def legacyShiftRightWidth: Boolean = dynamicContextVar.value.map(_.legacyShiftRightWidth).getOrElse(false)
def useLegacyWidth: Boolean = dynamicContextVar.value.map(_.useLegacyWidth).getOrElse(false)

// Builds a RenameMap for all Views that do not correspond to a single Data
// These Data give a fake ReferenceTarget for .toTarget and .toReferenceTarget that the returned
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
new DynamicContext(
annotationsInAspect,
chiselOptions.throwOnFirstError,
chiselOptions.useLegacyShiftRightWidth,
chiselOptions.useLegacyWidth,
chiselOptions.warningFilters,
chiselOptions.sourceRoots,
None,
Expand Down
29 changes: 19 additions & 10 deletions src/main/scala/chisel3/stage/ChiselAnnotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -362,28 +362,37 @@ object ChiselOutputFileAnnotation extends HasShellOptions {
*/
case class DesignAnnotation[DUT <: RawModule](design: DUT) extends NoTargetAnnotation with Unserializable

/** Use legacy Chisel shift-right behavior
/** Use legacy Chisel width behavior.
*
* '''This should only be used for checking for unexpected semantic changes when bumping to Chisel 7.0.0'''
*
* Use as CLI option `--use-legacy-shift-right-width`.
* Use as CLI option `--use-legacy-width`.
*
* This behavior is inconsistent between Chisel and FIRRTL
* - Chisel will report the width of a UInt or SInt shifted right by a number >= its width as a 0-bit value
* - FIRRTL will implement the width for these UInts and SInts as 1-bit
* There are two width bugs fixed in Chisel 7.0 that could affect the semantics of user code.
*
* 1. The width of shift-right when shift amount is >= the width of the argument
*
* This behavior is inconsistent between Chisel and FIRRTL.
* - Chisel will report the width of a UInt or SInt shifted right by a number >= its width as a 0-bit value.
* - FIRRTL will implement the width for these UInts and SInts as 1-bit.
*
* 2. The width of `ChiselEnum` values
*
* This behavior is inconsistent between the width reported by Chisel and the FIRRTL emitted by Chisel.
* - Calling .getWidth of a ChiselEnum value will give the width needed to encode the enum.
* - The resulting FIRRTL will have the minimum width needed to encode the literal value for that enum value.
*/
case object UseLegacyShiftRightWidthBehavior
case object UseLegacyWidthBehavior
extends NoTargetAnnotation
with ChiselOption
with HasShellOptions
with Unserializable {

val options = Seq(
new ShellOption[Unit](
longOption = "use-legacy-shift-right-width",
toAnnotationSeq = _ => Seq(UseLegacyShiftRightWidthBehavior),
helpText = "Use legacy (buggy) shift right width behavior (pre Chisel 7.0.0)"
longOption = "use-legacy-width",
toAnnotationSeq = _ => Seq(UseLegacyWidthBehavior),
helpText = "Use legacy (buggy) width behavior (pre Chisel 7.0.0)"
)
)

}
30 changes: 15 additions & 15 deletions src/main/scala/chisel3/stage/ChiselOptions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,22 @@ import chisel3.internal.WarningFilter
import java.io.File

class ChiselOptions private[stage] (
val printFullStackTrace: Boolean = false,
val throwOnFirstError: Boolean = false,
val outputFile: Option[String] = None,
val chiselCircuit: Option[Circuit] = None,
val sourceRoots: Vector[File] = Vector.empty,
val warningFilters: Vector[WarningFilter] = Vector.empty,
val useLegacyShiftRightWidth: Boolean = false) {
val printFullStackTrace: Boolean = false,
val throwOnFirstError: Boolean = false,
val outputFile: Option[String] = None,
val chiselCircuit: Option[Circuit] = None,
val sourceRoots: Vector[File] = Vector.empty,
val warningFilters: Vector[WarningFilter] = Vector.empty,
val useLegacyWidth: Boolean = false) {

private[stage] def copy(
printFullStackTrace: Boolean = printFullStackTrace,
throwOnFirstError: Boolean = throwOnFirstError,
outputFile: Option[String] = outputFile,
chiselCircuit: Option[Circuit] = chiselCircuit,
sourceRoots: Vector[File] = sourceRoots,
warningFilters: Vector[WarningFilter] = warningFilters,
useLegacyShiftRightWidth: Boolean = useLegacyShiftRightWidth
printFullStackTrace: Boolean = printFullStackTrace,
throwOnFirstError: Boolean = throwOnFirstError,
outputFile: Option[String] = outputFile,
chiselCircuit: Option[Circuit] = chiselCircuit,
sourceRoots: Vector[File] = sourceRoots,
warningFilters: Vector[WarningFilter] = warningFilters,
useLegacyWidth: Boolean = useLegacyWidth
): ChiselOptions = {

new ChiselOptions(
Expand All @@ -32,7 +32,7 @@ class ChiselOptions private[stage] (
chiselCircuit = chiselCircuit,
sourceRoots = sourceRoots,
warningFilters = warningFilters,
useLegacyShiftRightWidth = useLegacyShiftRightWidth
useLegacyWidth = useLegacyWidth
)

}
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/chisel3/stage/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ package object stage {
case SourceRootAnnotation(s) => c.copy(sourceRoots = c.sourceRoots :+ s)
case a: WarningConfigurationAnnotation => c.copy(warningFilters = c.warningFilters ++ a.filters)
case a: WarningConfigurationFileAnnotation => c.copy(warningFilters = c.warningFilters ++ a.filters)
case UseLegacyShiftRightWidthBehavior => c.copy(useLegacyShiftRightWidth = true)
case UseLegacyWidthBehavior => c.copy(useLegacyWidth = true)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/chisel3/stage/phases/Elaborate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class Elaborate extends Phase {
new DynamicContext(
annotations,
chiselOptions.throwOnFirstError,
chiselOptions.useLegacyShiftRightWidth,
chiselOptions.useLegacyWidth,
chiselOptions.warningFilters,
chiselOptions.sourceRoots,
None,
Expand Down
4 changes: 2 additions & 2 deletions src/main/scala/circt/stage/Shell.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import chisel3.stage.{
PrintFullStackTraceAnnotation,
SourceRootAnnotation,
ThrowOnFirstErrorAnnotation,
UseLegacyShiftRightWidthBehavior,
UseLegacyWidthBehavior,
WarningConfigurationAnnotation,
WarningConfigurationFileAnnotation,
WarningsAsErrorsAnnotation
Expand All @@ -36,7 +36,7 @@ trait CLI { this: BareShell =>
ChiselGeneratorAnnotation,
PrintFullStackTraceAnnotation,
ThrowOnFirstErrorAnnotation,
UseLegacyShiftRightWidthBehavior,
UseLegacyWidthBehavior,
WarningsAsErrorsAnnotation,
WarningConfigurationAnnotation,
WarningConfigurationFileAnnotation,
Expand Down
30 changes: 27 additions & 3 deletions src/test/scala/chiselTests/ChiselEnum.scala
Original file line number Diff line number Diff line change
Expand Up @@ -401,9 +401,8 @@ class ChiselEnumSpec extends ChiselFlatSpec with Utils {
assertTesterPasses(new CastToUIntTester)
}

// This is a bug, but fixing it may break user code.
// See: https://github.com/chipsalliance/chisel/issues/4159
it should "preserve legacy width behavior" in {
it should "give the correct width for Chisel Enum values" in {
val verilog = ChiselStage.emitSystemVerilog(new RawModule {
val out1, out2, out3 = IO(Output(UInt(8.W)))
val e = EnumExample.e1
Expand All @@ -413,12 +412,37 @@ class ChiselEnumSpec extends ChiselFlatSpec with Utils {
out1 := Cat(1.U, x)
out2 := Cat(1.U, y)
out3 := Cat(1.U, z)
// The bug is that the width of x is 7 but the value of out1 is 3
x.getWidth should be(7)
x.getWidth should be(EnumExample.getWidth)
y.widthOption should be(None)
z.getWidth should be(7)
})
verilog should include("assign out1 = 8'h81;")
verilog should include("assign out2 = 8'h81;")
verilog should include("assign out3 = 8'h81;")
}

// This is a bug, but fixing it may break user code.
// See: https://github.com/chipsalliance/chisel/issues/4159
it should "preserve legacy width behavior with --use-legacy-width" in {
val verilog = ChiselStage.emitSystemVerilog(
new RawModule {
val out1, out2, out3 = IO(Output(UInt(8.W)))
val e = EnumExample.e1
val x = e.asUInt
val y = e.asTypeOf(UInt())
val z = e.asTypeOf(UInt(e.getWidth.W))
out1 := Cat(1.U, x)
out2 := Cat(1.U, y)
out3 := Cat(1.U, z)
// The bug is that the width of x is 7 but the value of out1 is 3
x.getWidth should be(7)
x.getWidth should be(EnumExample.getWidth)
y.widthOption should be(None)
z.getWidth should be(7)
},
args = Array("--use-legacy-width")
)
// The bug is that all of these should be the same as out3, or the widths above are wrong
verilog should include("assign out1 = 8'h3;")
verilog should include("assign out2 = 8'h3;")
Expand Down
4 changes: 2 additions & 2 deletions src/test/scala/chiselTests/SIntOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,8 @@ class SIntOpsSpec extends ChiselPropSpec with Utils with ShiftRightWidthBehavior
testShiftRightWidthBehavior(SInt)(chiselMinWidth = 1, firrtlMinWidth = 1)
}

property("Static right-shift should have width of 0 in Chisel and 1 in FIRRTL with --use-legacy-shift-right-width") {
val args = Array("--use-legacy-shift-right-width")
property("Static right-shift should have width of 0 in Chisel and 1 in FIRRTL with --use-legacy-width") {
val args = Array("--use-legacy-width")

testShiftRightWidthBehavior(SInt)(chiselMinWidth = 0, firrtlMinWidth = 1, args = args)

Expand Down
8 changes: 4 additions & 4 deletions src/test/scala/chiselTests/UIntOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,8 @@ class UIntOpsSpec extends ChiselPropSpec with Matchers with Utils with ShiftRigh
testShiftRightWidthBehavior(UInt)(chiselMinWidth = 0, firrtlMinWidth = 0)
}

property("Static right-shift should have width of 0 in Chisel and 1 in FIRRTL with --use-legacy-shift-right-width") {
val args = Array("--use-legacy-shift-right-width")
property("Static right-shift should have width of 0 in Chisel and 1 in FIRRTL with --use-legacy-width") {
val args = Array("--use-legacy-width")

testShiftRightWidthBehavior(UInt)(chiselMinWidth = 0, firrtlMinWidth = 1, args = args)

Expand All @@ -570,7 +570,7 @@ class UIntOpsSpec extends ChiselPropSpec with Matchers with Utils with ShiftRigh
verilog should include(" widthcheck = 1'h0;")
}

property("--use-legacy-shift-right-width should have a minimal impact on emission") {
property("--use-legacy-width should have a minimal impact on emission") {
class TestModule extends Module {
val a, b, c = IO(Input(UInt(8.W)))
val widthcheck = Wire(UInt())
Expand All @@ -580,7 +580,7 @@ class UIntOpsSpec extends ChiselPropSpec with Matchers with Utils with ShiftRigh
widthcheck := (w >> 3) + b - c
}
val defaultFirrtl = ChiselStage.emitCHIRRTL(new TestModule)
val withOptFirrtl = ChiselStage.emitCHIRRTL(new TestModule, Array("--use-legacy-shift-right-width"))
val withOptFirrtl = ChiselStage.emitCHIRRTL(new TestModule, Array("--use-legacy-width"))
// We should see the fixup
val defaultOnly = Seq("node _widthcheck_T = shr(w, 3)")
val withOptOnly = Seq(
Expand Down

0 comments on commit b871e69

Please sign in to comment.