From 04caf395c737450c26f59d373d76b567a2b80f0f Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Thu, 1 Jul 2021 16:33:38 -0700 Subject: Change Chisel warnings to use logger instead of println It also uses the same logger as the Builder so that if we ever refactor that to be passed as an argument, it will be the same logger for both Builder and warning reporting. --- core/src/main/scala/chisel3/internal/Builder.scala | 2 +- core/src/main/scala/chisel3/internal/Error.scala | 25 +++++++++++----------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 35c4bdf9..fef12093 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -652,7 +652,7 @@ private[chisel3] object Builder extends LazyLogging { logger.warn("Elaborating design...") val mod = f mod.forceName(None, mod.name, globalNamespace) - errors.checkpoint() + errors.checkpoint(logger) logger.warn("Done elaborating.") (Circuit(components.last.name, components.toSeq, annotations.toSeq), mod) diff --git a/core/src/main/scala/chisel3/internal/Error.scala b/core/src/main/scala/chisel3/internal/Error.scala index 454be360..d6e0c0e6 100644 --- a/core/src/main/scala/chisel3/internal/Error.scala +++ b/core/src/main/scala/chisel3/internal/Error.scala @@ -4,6 +4,7 @@ package chisel3.internal import scala.annotation.tailrec import scala.collection.mutable.{ArrayBuffer, LinkedHashMap} +import _root_.logger.Logger object ExceptionHelpers { @@ -184,31 +185,31 @@ private[chisel3] class ErrorLog { } /** Throw an exception if any errors have yet occurred. */ - def checkpoint(): Unit = { + def checkpoint(logger: Logger): Unit = { deprecations.foreach { case ((message, sourceLoc), count) => - println(s"${ErrorLog.depTag} $sourceLoc ($count calls): $message") + logger.warn(s"${ErrorLog.depTag} $sourceLoc ($count calls): $message") } - errors foreach println + errors.foreach(e => logger.error(e.toString)) if (!deprecations.isEmpty) { - println(s"${ErrorLog.warnTag} ${Console.YELLOW}There were ${deprecations.size} deprecated function(s) used." + + logger.warn(s"${ErrorLog.warnTag} ${Console.YELLOW}There were ${deprecations.size} deprecated function(s) used." + s" These may stop compiling in a future release - you are encouraged to fix these issues.${Console.RESET}") - println(s"${ErrorLog.warnTag} Line numbers for deprecations reported by Chisel may be inaccurate; enable scalac compiler deprecation warnings via either of the following methods:") - println(s"${ErrorLog.warnTag} In the sbt interactive console, enter:") - println(s"""${ErrorLog.warnTag} set scalacOptions in ThisBuild ++= Seq("-unchecked", "-deprecation")""") - println(s"${ErrorLog.warnTag} or, in your build.sbt, add the line:") - println(s"""${ErrorLog.warnTag} scalacOptions := Seq("-unchecked", "-deprecation")""") + logger.warn(s"${ErrorLog.warnTag} Line numbers for deprecations reported by Chisel may be inaccurate; enable scalac compiler deprecation warnings via either of the following methods:") + logger.warn(s"${ErrorLog.warnTag} In the sbt interactive console, enter:") + logger.warn(s"""${ErrorLog.warnTag} set scalacOptions in ThisBuild ++= Seq("-unchecked", "-deprecation")""") + logger.warn(s"${ErrorLog.warnTag} or, in your build.sbt, add the line:") + logger.warn(s"""${ErrorLog.warnTag} scalacOptions := Seq("-unchecked", "-deprecation")""") } val allErrors = errors.filter(_.isFatal) val allWarnings = errors.filter(!_.isFatal) if (!allWarnings.isEmpty && !allErrors.isEmpty) { - println(s"${ErrorLog.errTag} There were ${Console.RED}${allErrors.size} error(s)${Console.RESET} and ${Console.YELLOW}${allWarnings.size} warning(s)${Console.RESET} during hardware elaboration.") + logger.warn(s"${ErrorLog.errTag} There were ${Console.RED}${allErrors.size} error(s)${Console.RESET} and ${Console.YELLOW}${allWarnings.size} warning(s)${Console.RESET} during hardware elaboration.") } else if (!allWarnings.isEmpty) { - println(s"${ErrorLog.warnTag} There were ${Console.YELLOW}${allWarnings.size} warning(s)${Console.RESET} during hardware elaboration.") + logger.warn(s"${ErrorLog.warnTag} There were ${Console.YELLOW}${allWarnings.size} warning(s)${Console.RESET} during hardware elaboration.") } else if (!allErrors.isEmpty) { - println(s"${ErrorLog.errTag} There were ${Console.RED}${allErrors.size} error(s)${Console.RESET} during hardware elaboration.") + logger.warn(s"${ErrorLog.errTag} There were ${Console.RED}${allErrors.size} error(s)${Console.RESET} during hardware elaboration.") } if (!allErrors.isEmpty) { -- cgit v1.2.3 From 5fe539c707c88eedbb112f5c6bcea1dfe1d52169 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Thu, 1 Jul 2021 16:34:48 -0700 Subject: Add ChiselEnum.safe factory method and avoid warning Previously, ChiselEnum would warn any time a UInt is converted to an Enum. There was no way to suppress this warning. Now there is a factory method (`.safe`) that does not warn and returns (Enum, Bool) where the Bool is the result of calling .isValid on an Enum object. The regular UInt cast is also now smarter and will not warn if all bitvectors of the width of the Enum are legal states. --- core/src/main/scala/chisel3/StrongEnum.scala | 34 ++++++++-- src/test/scala/chiselTests/ChiselSpec.scala | 17 ++++- src/test/scala/chiselTests/StrongEnum.scala | 97 ++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+), 5 deletions(-) diff --git a/core/src/main/scala/chisel3/StrongEnum.scala b/core/src/main/scala/chisel3/StrongEnum.scala index 7d328eb7..1d0e04d3 100644 --- a/core/src/main/scala/chisel3/StrongEnum.scala +++ b/core/src/main/scala/chisel3/StrongEnum.scala @@ -131,7 +131,7 @@ abstract class EnumType(private val factory: EnumFactory, selfAnnotating: Boolea if (litOption.isDefined) { true.B } else { - factory.all.map(this === _).reduce(_ || _) + if (factory.isTotal) true.B else factory.all.map(this === _).reduce(_ || _) } } @@ -233,6 +233,12 @@ abstract class EnumFactory { private[chisel3] val enumTypeName = getClass.getName.init + // Do all bitvectors of this Enum's width represent legal states? + private[chisel3] def isTotal: Boolean = { + (this.getWidth < 31) && // guard against Integer overflow + (enumRecords.size == (1 << this.getWidth)) + } + private[chisel3] def globalAnnotation: EnumDefChiselAnnotation = EnumDefChiselAnnotation(enumTypeName, (enumNames, enumValues).zipped.toMap) @@ -277,7 +283,7 @@ abstract class EnumFactory { def apply(): Type = new Type - def apply(n: UInt)(implicit sourceInfo: SourceInfo, connectionCompileOptions: CompileOptions): Type = { + private def castImpl(n: UInt, warn: Boolean)(implicit sourceInfo: SourceInfo, connectionCompileOptions: CompileOptions): Type = { if (n.litOption.isDefined) { enumInstances.find(_.litValue == n.litValue) match { case Some(result) => result @@ -288,8 +294,9 @@ abstract class EnumFactory { } else if (n.getWidth > this.getWidth) { throwException(s"The UInt being cast to $enumTypeName is wider than $enumTypeName's width ($getWidth)") } else { - Builder.warning(s"Casting non-literal UInt to $enumTypeName. You can check that its value is legal by calling isValid") - + if (warn && !this.isTotal) { + Builder.warning(s"Casting non-literal UInt to $enumTypeName. You can use $enumTypeName.safe to cast without this warning.") + } val glue = Wire(new UnsafeEnum(width)) glue := n val result = Wire(new Type) @@ -297,6 +304,25 @@ abstract class EnumFactory { result } } + + /** Cast an [[UInt]] to the type of this Enum + * + * @note will give a Chisel elaboration time warning if the argument could hit invalid states + * @param n the UInt to cast + * @return the equivalent Enum to the value of the cast UInt + */ + def apply(n: UInt)(implicit sourceInfo: SourceInfo, connectionCompileOptions: CompileOptions): Type = castImpl(n, warn = true) + + /** Safely cast an [[UInt]] to the type of this Enum + * + * @param n the UInt to cast + * @return the equivalent Enum to the value of the cast UInt and a Bool indicating if the + * Enum is valid + */ + def safe(n: UInt)(implicit sourceInfo: SourceInfo, connectionCompileOptions: CompileOptions): (Type, Bool) = { + val t = castImpl(n, warn = false) + (t, t.isValid) + } } diff --git a/src/test/scala/chiselTests/ChiselSpec.scala b/src/test/scala/chiselTests/ChiselSpec.scala index a4192c5e..e513189e 100644 --- a/src/test/scala/chiselTests/ChiselSpec.scala +++ b/src/test/scala/chiselTests/ChiselSpec.scala @@ -9,6 +9,7 @@ import chisel3.testers._ import firrtl.annotations.Annotation import firrtl.util.BackendCompilationUtilities import firrtl.{AnnotationSeq, EmittedVerilogCircuitAnnotation} +import _root_.logger.Logger import org.scalacheck._ import org.scalatest._ import org.scalatest.flatspec.AnyFlatSpec @@ -17,7 +18,7 @@ import org.scalatest.propspec.AnyPropSpec import org.scalatest.matchers.should.Matchers import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks -import java.io.ByteArrayOutputStream +import java.io.{ByteArrayOutputStream, PrintStream} import java.security.Permission import scala.reflect.ClassTag @@ -172,6 +173,20 @@ trait Utils { (stdout.toString, stderr.toString, ret) } + /** Run some Scala thunk and return all logged messages as Strings + * @param thunk some Scala code + * @return a tuple containing LOGGED, and what the thunk returns + */ + def grabLog[T](thunk: => T): (String, T) = { + val baos = new ByteArrayOutputStream() + val stream = new PrintStream(baos, true, "utf-8") + val ret = Logger.makeScope(Nil) { + Logger.setOutput(stream) + thunk + } + (baos.toString, ret) + } + /** Encodes a System.exit exit code * @param status the exit code */ diff --git a/src/test/scala/chiselTests/StrongEnum.scala b/src/test/scala/chiselTests/StrongEnum.scala index bf0eb2fe..e59a5398 100644 --- a/src/test/scala/chiselTests/StrongEnum.scala +++ b/src/test/scala/chiselTests/StrongEnum.scala @@ -74,6 +74,18 @@ class CastFromNonLit extends Module { io.valid := io.out.isValid } +class SafeCastFromNonLit extends Module { + val io = IO(new Bundle { + val in = Input(UInt(EnumExample.getWidth.W)) + val out = Output(EnumExample()) + val valid = Output(Bool()) + }) + + val (enum, valid) = EnumExample.safe(io.in) + io.out := enum + io.valid := valid +} + class CastFromNonLitWidth(w: Option[Int] = None) extends Module { val width = if (w.isDefined) w.get.W else UnknownWidth() @@ -191,6 +203,28 @@ class CastFromNonLitTester extends BasicTester { stop() } +class SafeCastFromNonLitTester extends BasicTester { + for ((enum,lit) <- EnumExample.all zip EnumExample.litValues) { + val mod = Module(new SafeCastFromNonLit) + mod.io.in := lit + assert(mod.io.out === enum) + assert(mod.io.valid === true.B) + } + + val invalid_values = (1 until (1 << EnumExample.getWidth)). + filter(!EnumExample.litValues.map(_.litValue).contains(_)). + map(_.U) + + for (invalid_val <- invalid_values) { + val mod = Module(new SafeCastFromNonLit) + mod.io.in := invalid_val + + assert(mod.io.valid === false.B) + } + + stop() +} + class CastToInvalidEnumTester extends BasicTester { val invalid_value: UInt = EnumExample.litValues.last + 1.U Module(new CastFromLit(invalid_value)) @@ -320,6 +354,10 @@ class StrongEnumSpec extends ChiselFlatSpec with Utils { assertTesterPasses(new CastFromNonLitTester) } + it should "safely cast non-literal UInts to enums correctly and detect illegal casts" in { + assertTesterPasses(new SafeCastFromNonLitTester) + } + it should "prevent illegal literal casts to enums" in { a [ChiselException] should be thrownBy extractCause[ChiselException] { ChiselStage.elaborate(new CastToInvalidEnumTester) @@ -377,6 +415,65 @@ class StrongEnumSpec extends ChiselFlatSpec with Utils { "StrongEnum FSM" should "work" in { assertTesterPasses(new StrongEnumFSMTester) } + + "Casting a UInt to an Enum" should "warn if the UInt can express illegal states" in { + object MyEnum extends ChiselEnum { + val e0, e1, e2 = Value + } + + class MyModule extends Module { + val in = IO(Input(UInt(2.W))) + val out = IO(Output(MyEnum())) + out := MyEnum(in) + } + val (log, _) = grabLog(ChiselStage.elaborate(new MyModule)) + log should include ("warn") + log should include ("Casting non-literal UInt") + } + + it should "NOT warn if the Enum is total" in { + object TotalEnum extends ChiselEnum { + val e0, e1, e2, e3 = Value + } + + class MyModule extends Module { + val in = IO(Input(UInt(2.W))) + val out = IO(Output(TotalEnum())) + out := TotalEnum(in) + } + val (log, _) = grabLog(ChiselStage.elaborate(new MyModule)) + log should not include ("warn") + } + + "Casting a UInt to an Enum with .safe" should "NOT warn" in { + object MyEnum extends ChiselEnum { + val e0, e1, e2 = Value + } + + class MyModule extends Module { + val in = IO(Input(UInt(2.W))) + val out = IO(Output(MyEnum())) + out := MyEnum.safe(in)._1 + } + val (log, _) = grabLog(ChiselStage.elaborate(new MyModule)) + log should not include ("warn") + } + + it should "NOT generate any validity logic if the Enum is total" in { + object TotalEnum extends ChiselEnum { + val e0, e1, e2, e3 = Value + } + + class MyModule extends Module { + val in = IO(Input(UInt(2.W))) + val out = IO(Output(TotalEnum())) + val (res, valid) = TotalEnum.safe(in) + assert(valid.litToBoolean, "It should be true.B") + out := res + } + val (log, _) = grabLog(ChiselStage.elaborate(new MyModule)) + log should not include ("warn") + } } class StrongEnumAnnotator extends Module { -- cgit v1.2.3 From bfb77d4cc1163e34cc54ce856214a0181b2cb029 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Thu, 1 Jul 2021 18:03:55 -0700 Subject: Update docs for ChiselEnum --- docs/src/explanations/chisel-enum.md | 170 ++++++++++++++++++++++++----------- 1 file changed, 118 insertions(+), 52 deletions(-) diff --git a/docs/src/explanations/chisel-enum.md b/docs/src/explanations/chisel-enum.md index cb017239..3f0f3b18 100644 --- a/docs/src/explanations/chisel-enum.md +++ b/docs/src/explanations/chisel-enum.md @@ -1,4 +1,4 @@ ---- +--- layout: docs title: "Enumerations" section: "chisel3" @@ -6,8 +6,8 @@ section: "chisel3" # ChiselEnum -The ChiselEnum type can be used to reduce the chance of error when encoding mux selectors, opcodes, and functional unit operations. In contrast with`Chisel.util.Enum`, `ChiselEnum` are subclasses of `Data`, which means that they can be used to define fields in `Bundle`s, including in `IO`s. - +The ChiselEnum type can be used to reduce the chance of error when encoding mux selectors, opcodes, and functional unit operations. +In contrast with `Chisel.util.Enum`, `ChiselEnum` are subclasses of `Data`, which means that they can be used to define fields in `Bundle`s, including in `IO`s. ## Functionality and Examples @@ -19,57 +19,68 @@ import chisel3.stage.ChiselStage import chisel3.experimental.ChiselEnum ``` -Below we see ChiselEnum being used as mux select for a RISC-V core. While wrapping the object in a package is not required, it is highly recommended as it allows for the type to be used in multiple files more easily. +```scala mdoc:invisible +// Helper to print stdout from Chisel elab +// May be related to: https://github.com/scalameta/mdoc/issues/517 +import java.io._ +import _root_.logger.Logger +def grabLog[T](thunk: => T): (String, T) = { + val baos = new ByteArrayOutputStream() + val stream = new PrintStream(baos, true, "utf-8") + val ret = Logger.makeScope(Nil) { + Logger.setOutput(stream) + thunk + } + (baos.toString, ret) +} +``` + +Below we see ChiselEnum being used as mux select for a RISC-V core. While wrapping the object in a package is not required, it is highly recommended as it allows for the type to be used in multiple files more easily. ```scala mdoc // package CPUTypes { - object AluMux1Sel extends ChiselEnum { - val selectRS1, selectPC = Value - /** How the values will be mapped - "selectRS1" -> 0.U, - "selectPC" -> 1.U - */ - } -// } +object AluMux1Sel extends ChiselEnum { + val selectRS1, selectPC = Value +} +// We can see the mapping by printing each Value +AluMux1Sel.all.foreach(println) ``` -Here we see a mux using the AluMux1Sel to select between different inputs. +Here we see a mux using the AluMux1Sel to select between different inputs. ```scala mdoc import AluMux1Sel._ class AluMux1Bundle extends Bundle { - val aluMux1Sel = Input( AluMux1Sel() ) - val rs1Out = Input(Bits(32.W)) - val pcOut = Input(Bits(32.W)) - val aluMux1Out = Output(Bits(32.W)) + val aluMux1Sel = Input(AluMux1Sel()) + val rs1Out = Input(Bits(32.W)) + val pcOut = Input(Bits(32.W)) + val aluMux1Out = Output(Bits(32.W)) } class AluMux1File extends Module { - val io = IO(new AluMux1Bundle) - - // Default value for aluMux1Out - io.aluMux1Out := 0.U - - switch (io.aluMux1Sel) { - is (selectRS1) { - io.aluMux1Out := io.rs1Out - } - is (selectPC) { - io.aluMux1Out := io.pcOut - } + val io = IO(new AluMux1Bundle) + + // Default value for aluMux1Out + io.aluMux1Out := 0.U + + switch (io.aluMux1Sel) { + is (selectRS1) { + io.aluMux1Out := io.rs1Out } + is (selectPC) { + io.aluMux1Out := io.pcOut + } + } } ``` ```scala mdoc:verilog -ChiselStage.emitVerilog(new AluMux1File ) +ChiselStage.emitVerilog(new AluMux1File) ``` -ChiselEnum also allows for the user to define variables by passing in the value shown below. Note that the value must be increasing or else - - > chisel3.internal.ChiselException: Exception thrown when elaborating ChiselGeneratorAnnotation - -is thrown during Verilog generation. +ChiselEnum also allows for the user to directly set the Values by passing an `UInt` to `Value(...)` +as shown below. Note that the magnitude of each `Value` must be strictly greater than the one before +it. ```scala mdoc object Opcode extends ChiselEnum { @@ -85,27 +96,81 @@ object Opcode extends ChiselEnum { } ``` -The user can 'jump' to a value and continue incrementing by passing a start point then using a regular Value assignment. +The user can 'jump' to a value and continue incrementing by passing a start point then using a regular Value definition. ```scala mdoc object BranchFunct3 extends ChiselEnum { val beq, bne = Value val blt = Value(4.U) val bge, bltu, bgeu = Value - /** How the values will be mapped - "beq" -> 0.U, - "bne" -> 1.U, - "blt" -> 4.U, - "bge" -> 5.U, - "bltu" -> 6.U, - "bgeu" -> 7.U - */ } +// We can see the mapping by printing each Value +BranchFunct3.all.foreach(println) +``` + +## Casting + +You can cast an enum to a `UInt` using `.asUInt`: + +```scala mdoc +class ToUInt extends RawModule { + val in = IO(Input(Opcode())) + val out = IO(Output(UInt())) + out := in.asUInt +} +``` + +```scala mdoc:invisible +// Always need to run Chisel to see if there are elaboration errors +ChiselStage.emitVerilog(new ToUInt) +``` + +You can cast from a `UInt` to an enum by passing the `UInt` to the apply method of the `ChiselEnum` object: + +```scala mdoc +class FromUInt extends Module { + val in = IO(Input(UInt(7.W))) + val out = IO(Output(Opcode())) + out := Opcode(in) +} +``` + +However, if you cast from a `UInt` to an Enum type when there are undefined states in the Enum values +that the `UInt` could hit, you will see a warning like the following: + +```scala mdoc:passthrough +val (log, _) = grabLog(ChiselStage.emitChirrtl(new FromUInt)) +println(s"```$log```") +``` +(Note that the name of the Enum is ugly as an artifact of our documentation generation flow, it will +be cleaner in normal use). + +You can avoid this warning by using the `.safe` factory method which returns the cast Enum in addition +to a `Bool` indicating if the Enum is in a valid state: + +```scala mdoc +class SafeFromUInt extends Module { + val in = IO(Input(UInt(7.W))) + val out = IO(Output(Opcode())) + val (value, valid) = Opcode.safe(in) + assert(valid, "Enum state must be valid, got %d!", in) + out := value +} +``` + +Now there will be no warning: + +```scala mdoc:passthrough +val (log2, _) = grabLog(ChiselStage.emitChirrtl(new SafeFromUInt)) +println(s"```$log2```") ``` ## Testing -When testing your modules, the `.Type` and `.litValue` attributes allow for the the objects to be passed as parameters and for the value to be converted to BigInt type. Note that BigInts cannot be casted to Int with `.asInstanceOf[Int]`, they use their own methods like `toInt`. Please review the [scala.math.BigInt](https://www.scala-lang.org/api/2.12.5/scala/math/BigInt.html) page for more details! +The _Type_ of the enums values is `.Type` which can be useful for passing the values +as parameters to a function (or any other time a type annotation is needed). +Calling `.litValue` on an enum value will return the integer value of that object as a +[`BigInt`](https://www.scala-lang.org/api/2.12.13/scala/math/BigInt.html). ```scala mdoc def expectedSel(sel: AluMux1Sel.Type): Boolean = sel match { @@ -115,22 +180,23 @@ def expectedSel(sel: AluMux1Sel.Type): Boolean = sel match { } ``` -The ChiselEnum type also has methods `.all` and `.getWidth` where `all` returns all of the enum instances and `getWidth` returns the width of the hardware type. +Some additional useful methods defined on the `ChiselEnum` object are: +* `.all`: returns the enum values within the enumeration +* `.getWidth`: returns the width of the hardware type ## Workarounds -As of 2/26/2021, the width of the values is always inferred. To work around this, you can add an extra `Value` that forces the width that is desired. This is shown in the example below, where we add a field `ukn` to force the width to be 3 bits wide: +As of Chisel v3.4.3 (1 July 2020), the width of the values is always inferred. +To work around this, you can add an extra `Value` that forces the width that is desired. +This is shown in the example below, where we add a field `ukn` to force the width to be 3 bits wide: ```scala mdoc object StoreFunct3 extends ChiselEnum { val sb, sh, sw = Value val ukn = Value(7.U) - /** How the values will be mapped - "sb" -> 0.U, - "sh" -> 1.U, - "sw" -> 2.U - */ } +// We can see the mapping by printing each Value +StoreFunct3.all.foreach(println) ``` Signed values are not supported so if you want the value signed, you must cast the UInt with `.asSInt`. -- cgit v1.2.3