Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix width of ChiselEnum values in emitted FIRRTL #4200

Merged
merged 2 commits into from
Jul 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -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
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
34 changes: 23 additions & 11 deletions src/main/scala/chisel3/stage/ChiselAnnotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -362,28 +362,40 @@ 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'''
* '''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.
jackkoenig marked this conversation as resolved.
Show resolved Hide resolved
* 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
*
* 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
*
* 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 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 @@ -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)

Expand All @@ -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())
Expand All @@ -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(
Expand Down