From 88a16abef48e45147ee066503aa6d65a5069d853 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Thu, 20 Jun 2024 17:01:56 -0700 Subject: [PATCH 1/2] Fix width of ChiselEnum values in emitted FIRRTL 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. --- core/src/main/scala/chisel3/Bits.scala | 4 +-- core/src/main/scala/chisel3/ChiselEnum.scala | 12 ++++++-- .../hierarchy/core/Definition.scala | 2 +- .../main/scala/chisel3/internal/Builder.scala | 14 ++++----- .../aop/injecting/InjectingAspect.scala | 2 +- .../chisel3/stage/ChiselAnnotations.scala | 29 +++++++++++------- .../scala/chisel3/stage/ChiselOptions.scala | 30 +++++++++---------- src/main/scala/chisel3/stage/package.scala | 2 +- .../chisel3/stage/phases/Elaborate.scala | 2 +- src/main/scala/circt/stage/Shell.scala | 4 +-- src/test/scala/chiselTests/ChiselEnum.scala | 30 +++++++++++++++++-- src/test/scala/chiselTests/SIntOps.scala | 4 +-- src/test/scala/chiselTests/UIntOps.scala | 8 ++--- 13 files changed, 92 insertions(+), 51 deletions(-) diff --git a/core/src/main/scala/chisel3/Bits.scala b/core/src/main/scala/chisel3/Bits.scala index d3bb863e7fe..56b15fb9d75 100644 --- a/core/src/main/scala/chisel3/Bits.scala +++ b/core/src/main/scala/chisel3/Bits.scala @@ -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 = @@ -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 = diff --git a/core/src/main/scala/chisel3/ChiselEnum.scala b/core/src/main/scala/chisel3/ChiselEnum.scala index ce4dfeddf41..bd357aa8f81 100644 --- a/core/src/main/scala/chisel3/ChiselEnum.scala +++ b/core/src/main/scala/chisel3/ChiselEnum.scala @@ -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 diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala index e4bb2548fc4..21800326448 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala @@ -116,7 +116,7 @@ object Definition extends SourceInfoDoc { new DynamicContext( Nil, context.throwOnFirstError, - context.legacyShiftRightWidth, + context.useLegacyWidth, context.warningFilters, context.sourceRoots, Some(context.globalNamespace), diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index d3e2ba98d4f..5f38b2ee632 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -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[_]], @@ -955,7 +955,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 diff --git a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala index 0c11dbf8e68..7d92bac8ed5 100644 --- a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala +++ b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala @@ -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, diff --git a/src/main/scala/chisel3/stage/ChiselAnnotations.scala b/src/main/scala/chisel3/stage/ChiselAnnotations.scala index 0beeef2a8c4..982ed5d09c0 100644 --- a/src/main/scala/chisel3/stage/ChiselAnnotations.scala +++ b/src/main/scala/chisel3/stage/ChiselAnnotations.scala @@ -362,17 +362,27 @@ 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 @@ -380,10 +390,9 @@ case object UseLegacyShiftRightWidthBehavior 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)" ) ) - } diff --git a/src/main/scala/chisel3/stage/ChiselOptions.scala b/src/main/scala/chisel3/stage/ChiselOptions.scala index 336e92ebd09..f7af0579188 100644 --- a/src/main/scala/chisel3/stage/ChiselOptions.scala +++ b/src/main/scala/chisel3/stage/ChiselOptions.scala @@ -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( @@ -32,7 +32,7 @@ class ChiselOptions private[stage] ( chiselCircuit = chiselCircuit, sourceRoots = sourceRoots, warningFilters = warningFilters, - useLegacyShiftRightWidth = useLegacyShiftRightWidth + useLegacyWidth = useLegacyWidth ) } diff --git a/src/main/scala/chisel3/stage/package.scala b/src/main/scala/chisel3/stage/package.scala index 90ae9c9dcef..e7781883ce1 100644 --- a/src/main/scala/chisel3/stage/package.scala +++ b/src/main/scala/chisel3/stage/package.scala @@ -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) } } diff --git a/src/main/scala/chisel3/stage/phases/Elaborate.scala b/src/main/scala/chisel3/stage/phases/Elaborate.scala index 3ddf4edeecd..fe6f766ee19 100644 --- a/src/main/scala/chisel3/stage/phases/Elaborate.scala +++ b/src/main/scala/chisel3/stage/phases/Elaborate.scala @@ -41,7 +41,7 @@ class Elaborate extends Phase { new DynamicContext( annotations, chiselOptions.throwOnFirstError, - chiselOptions.useLegacyShiftRightWidth, + chiselOptions.useLegacyWidth, chiselOptions.warningFilters, chiselOptions.sourceRoots, None, diff --git a/src/main/scala/circt/stage/Shell.scala b/src/main/scala/circt/stage/Shell.scala index f2295b86f7c..b6820cf0dd2 100644 --- a/src/main/scala/circt/stage/Shell.scala +++ b/src/main/scala/circt/stage/Shell.scala @@ -9,7 +9,7 @@ import chisel3.stage.{ PrintFullStackTraceAnnotation, SourceRootAnnotation, ThrowOnFirstErrorAnnotation, - UseLegacyShiftRightWidthBehavior, + UseLegacyWidthBehavior, WarningConfigurationAnnotation, WarningConfigurationFileAnnotation, WarningsAsErrorsAnnotation @@ -36,7 +36,7 @@ trait CLI { this: BareShell => ChiselGeneratorAnnotation, PrintFullStackTraceAnnotation, ThrowOnFirstErrorAnnotation, - UseLegacyShiftRightWidthBehavior, + UseLegacyWidthBehavior, WarningsAsErrorsAnnotation, WarningConfigurationAnnotation, WarningConfigurationFileAnnotation, diff --git a/src/test/scala/chiselTests/ChiselEnum.scala b/src/test/scala/chiselTests/ChiselEnum.scala index a22b9016367..5481ec97944 100644 --- a/src/test/scala/chiselTests/ChiselEnum.scala +++ b/src/test/scala/chiselTests/ChiselEnum.scala @@ -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 @@ -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;") diff --git a/src/test/scala/chiselTests/SIntOps.scala b/src/test/scala/chiselTests/SIntOps.scala index 12a3086d20d..13c73db48dd 100644 --- a/src/test/scala/chiselTests/SIntOps.scala +++ b/src/test/scala/chiselTests/SIntOps.scala @@ -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) diff --git a/src/test/scala/chiselTests/UIntOps.scala b/src/test/scala/chiselTests/UIntOps.scala index 2c902cf865a..238d93befe3 100644 --- a/src/test/scala/chiselTests/UIntOps.scala +++ b/src/test/scala/chiselTests/UIntOps.scala @@ -557,8 +557,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) @@ -575,7 +575,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()) @@ -585,7 +585,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( From 5489b86d6df1e1caef69c4e84f547b060784229d Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Fri, 28 Jun 2024 16:42:26 -0700 Subject: [PATCH 2/2] Clarify UseLegacyWidthBehavior ScalaDoc --- .../chisel3/stage/ChiselAnnotations.scala | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/main/scala/chisel3/stage/ChiselAnnotations.scala b/src/main/scala/chisel3/stage/ChiselAnnotations.scala index 982ed5d09c0..68bdcbd7cfc 100644 --- a/src/main/scala/chisel3/stage/ChiselAnnotations.scala +++ b/src/main/scala/chisel3/stage/ChiselAnnotations.scala @@ -364,23 +364,26 @@ case class DesignAnnotation[DUT <: RawModule](design: DUT) extends NoTargetAnnot /** Use legacy Chisel width behavior. * - * '''This should only be used for checking for unexpected semantic changes when bumping to Chisel 7.0.0''' + * '''This should only be used for checking for unexpected semantic changes when bumping to Chisel 7.0.0.''' * * Use as CLI option `--use-legacy-width`. * * There are two width bugs fixed in Chisel 7.0 that could affect the semantics of user code. + * Enabling this option will restore the old, buggy behavior, described below: * * 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. + * When this option is enabled, the behavior is as follows: + * - Calling `.getWidth` on the resulting value will report the width as 0. + * - The width of the resulting value will be treated as 1-bit for generating Verilog. * * 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. + * When this option is enabled, the behavior is as follows: + * - Calling `.getWidth` on a specific ChiselEnum value will give the width needed to encode the enum. + * This is the minimum width needed to encode the maximum value encoded by the enum. + * - The resulting FIRRTL will have the minimum width needed to encode the literal value for just that specific + * enum value. */ case object UseLegacyWidthBehavior extends NoTargetAnnotation @@ -392,7 +395,7 @@ case object UseLegacyWidthBehavior new ShellOption[Unit]( longOption = "use-legacy-width", toAnnotationSeq = _ => Seq(UseLegacyWidthBehavior), - helpText = "Use legacy (buggy) width behavior (pre Chisel 7.0.0)" + helpText = "Use legacy (buggy) width behavior (pre-Chisel 7.0.0)" ) ) }