From ff26f51436cad7ed09fb5e6ee6b8a0241d142055 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Wed, 9 Jan 2019 00:04:15 -0500 Subject: Fix BoringUtils deduplication bug This fixes a bug where BoringUtils non-hierarchical sinks would be deduplicated even when specified that they should not be. h/t @ucbjrl for discovering this! Signed-off-by: Schuyler Eldridge --- src/main/scala/chisel3/util/experimental/BoringUtils.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/scala/chisel3/util/experimental/BoringUtils.scala b/src/main/scala/chisel3/util/experimental/BoringUtils.scala index 066e924f..c1bfa240 100644 --- a/src/main/scala/chisel3/util/experimental/BoringUtils.scala +++ b/src/main/scala/chisel3/util/experimental/BoringUtils.scala @@ -177,7 +177,7 @@ object BoringUtils { val annotations = Seq(new ChiselAnnotation with RunFirrtlTransform { def toFirrtl = SinkAnnotation(component.toNamed, name) - def transformClass = classOf[WiringTransform] }) + def transformClass = classOf[WiringTransform] }) ++ maybeDedup annotations.map(annotate(_)) } -- cgit v1.2.3 From 1f5acfbd1efb3f4dea9325be4206a263ab13009b Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Wed, 9 Jan 2019 00:07:02 -0500 Subject: Fix BoringUtilsSpec to require no dedup This adds two tests to the BoringUtilsSpec to explicitly verify that deduplication is required when boring. This adds tests that both verify that the test passes as expected with deduplication enabled and that the same test fails with deduplication disabled. Signed-off-by: Schuyler Eldridge --- src/test/scala/chiselTests/BoringUtilsSpec.scala | 71 ++++++++++++++++-------- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/src/test/scala/chiselTests/BoringUtilsSpec.scala b/src/test/scala/chiselTests/BoringUtilsSpec.scala index 3af3b4fa..06840577 100644 --- a/src/test/scala/chiselTests/BoringUtilsSpec.scala +++ b/src/test/scala/chiselTests/BoringUtilsSpec.scala @@ -7,11 +7,13 @@ import java.io.File import chisel3._ import chisel3.util.Counter import chisel3.testers.BasicTester -import chisel3.experimental.{MultiIOModule, RawModule, BaseModule} +import chisel3.experimental.{BaseModule, ChiselAnnotation, MultiIOModule, RawModule, RunFirrtlTransform} import chisel3.util.experimental.BoringUtils -import firrtl.{CommonOptions, ExecutionOptionsManager, HasFirrtlOptions, FirrtlExecutionOptions, FirrtlExecutionSuccess, - FirrtlExecutionFailure} -import firrtl.passes.wiring.WiringTransform + +import firrtl.{CircuitForm, CircuitState, ChirrtlForm, Transform} +import firrtl.annotations.NoTargetAnnotation +import firrtl.transforms.{DontTouchAnnotation, NoDedupAnnotation} +import firrtl.passes.wiring.WiringException abstract class ShouldntAssertTester(cyclesToWait: BigInt = 4) extends BasicTester { val dut: BaseModule @@ -19,6 +21,13 @@ abstract class ShouldntAssertTester(cyclesToWait: BigInt = 4) extends BasicTeste when (done) { stop() } } +class StripNoDedupAnnotation extends Transform { + def inputForm: CircuitForm = ChirrtlForm + def outputForm: CircuitForm = ChirrtlForm + def execute(state: CircuitState): CircuitState = + state.copy(annotations = state.annotations.filter{ case _: NoDedupAnnotation => false; case _ => true }) +} + class BoringUtilsSpec extends ChiselFlatSpec with ChiselRunners { class BoringInverter extends Module { @@ -42,37 +51,47 @@ class BoringUtilsSpec extends ChiselFlatSpec with ChiselRunners { trait WireX { this: BaseModule => val x = Wire(UInt(4.W)) + chisel3.experimental.annotate(new ChiselAnnotation{ def toFirrtl = DontTouchAnnotation(x.toNamed) }) } - class Constant(const: Int) extends MultiIOModule with WireX { - x := const.U + class Source extends RawModule with WireX { + val in = IO(Input(UInt())) + x := in } - object Constant { def apply(const: Int): Constant = Module(new Constant(const)) } - - class Expect(const: Int) extends MultiIOModule with WireX { + class Sink extends RawModule with WireX { + val out = IO(Output(UInt())) x := 0.U // Default value. Output is zero unless we bore... - chisel3.assert(x === const.U, "x (0x%x) was not const.U (0x%x)", x, const.U) + out := x } - object Expect { def apply(const: Int): Expect = Module(new Expect(const)) } - - // After boring, this will have the following connections: - // - source(0) -> unconnected - // - unconnected -> expect(0) - // - source(1) -> expect(1) - // - source(2) -> expect(2) class Top(val width: Int) extends MultiIOModule { - val source = Seq(0, 1, 2).map(x => x -> Constant(x)).toMap - val expect = Map(0 -> Seq.fill(2)(Expect(0)), - 1 -> Seq.fill(1)(Expect(1)), - 2 -> Seq.fill(3)(Expect(2))) + /* From the perspective of deduplication, all sources are identical and all sinks are identical. */ + val sources = Seq.fill(3)(Module(new Source)) + val sinks = Seq.fill(6)(Module(new Sink)) + + /* Sources are differentiated by their input connections only. */ + sources.zip(Seq(0, 1, 2)).map{ case (a, b) => a.in := b.U } + + /* Sinks are differentiated by their post-boring outputs. */ + sinks.zip(Seq(0, 1, 1, 2, 2, 2)).map{ case (a, b) => chisel3.assert(a.out === b.U) } } + /** This is testing a complicated wiring pattern and exercising the necessity of disabling deduplication for sources and + * sinks. Without disabling deduplication, this test will fail. + */ class TopTester extends ShouldntAssertTester { val dut = Module(new Top(4)) - BoringUtils.bore(dut.source(1).x, dut.expect(1).map(_.x)) - BoringUtils.bore(dut.source(2).x, dut.expect(2).map(_.x)) + BoringUtils.bore(dut.sources(1).x, Seq(dut.sinks(1).x, dut.sinks(2).x)) + BoringUtils.bore(dut.sources(2).x, Seq(dut.sinks(3).x, dut.sinks(4).x, dut.sinks(5).x)) + } + + trait FailViaDedup { this: TopTester => + case object FooAnnotation extends NoTargetAnnotation + chisel3.experimental.annotate( + new ChiselAnnotation with RunFirrtlTransform { + def toFirrtl = FooAnnotation + def transformClass = classOf[StripNoDedupAnnotation] } ) } behavior of "BoringUtils.bore" @@ -80,4 +99,10 @@ class BoringUtilsSpec extends ChiselFlatSpec with ChiselRunners { it should "connect across modules using BoringUtils.bore" in { runTester(new TopTester) should be (true) } + + it should "throw an exception if NoDedupAnnotations are removed" in { + intercept[WiringException] { runTester(new TopTester with FailViaDedup) } + .getMessage should startWith ("Unable to determine source mapping for sink") + } + } -- cgit v1.2.3 From 11236859f08c46c6df3249e7f04b1e4fdad75c20 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Wed, 9 Jan 2019 00:07:51 -0500 Subject: Changes to BoringUtils Scaladoc, paramater name This compresses the Scaladoc for BoringUtils slightly by using 120 character lines and removing unnecessary whitespace. This also changes the poorly named "dedup" parameter to the what it actually is: "disableDedup". Signed-off-by: Schuyler Eldridge --- .../chisel3/util/experimental/BoringUtils.scala | 95 ++++++++++------------ 1 file changed, 43 insertions(+), 52 deletions(-) diff --git a/src/main/scala/chisel3/util/experimental/BoringUtils.scala b/src/main/scala/chisel3/util/experimental/BoringUtils.scala index c1bfa240..67a6b6d0 100644 --- a/src/main/scala/chisel3/util/experimental/BoringUtils.scala +++ b/src/main/scala/chisel3/util/experimental/BoringUtils.scala @@ -17,14 +17,11 @@ import chisel3.internal.Namespace */ class BoringUtilsException(message: String) extends Exception(message) -/** Utilities for generating synthesizeable cross module references that - * "bore" through the hieararchy. The underlying cross module connects - * are handled by FIRRTL's Wiring Transform - * ([[firrtl.passes.wiring.WiringTransform]]). +/** Utilities for generating synthesizable cross module references that "bore" through the hierarchy. The underlying + * cross module connects are handled by FIRRTL's Wiring Transform ([[firrtl.passes.wiring.WiringTransform]]). * - * Consider the following exmple where you want to connect a component in - * one module to a component in another. Module `Constant` has a wire - * tied to `42` and `Expect` will assert unless connected to `42`: + * Consider the following exmple where you want to connect a component in one module to a component in another. Module + * `Constant` has a wire tied to `42` and `Expect` will assert unless connected to `42`: * {{{ * class Constant extends Module { * val io = IO(new Bundle{}) @@ -40,9 +37,8 @@ class BoringUtilsException(message: String) extends Exception(message) * } * }}} * - * We can then connect `x` to `y` using [[BoringUtils]] without - * modifiying the Chisel IO of `Constant`, `Expect`, or modules that may - * instantiate them. There are two approaches to do this: + * We can then connect `x` to `y` using [[BoringUtils]] without modifiying the Chisel IO of `Constant`, `Expect`, or + * modules that may instantiate them. There are two approaches to do this: * * 1. Hierarchical boring using [[BoringUtils.bore]] * @@ -50,10 +46,9 @@ class BoringUtilsException(message: String) extends Exception(message) * * ===Hierarchical Boring=== * - * Hierarchcical boring involves connecting one sink instance to another - * source instance in a parent module. Below, module `Top` contains an - * instance of `Cosntant` and `Expect`. Using [[BoringUtils.bore]], we - * can connect `constant.x` to `expect.y`. + * Hierarchcical boring involves connecting one sink instance to another source instance in a parent module. Below, + * module `Top` contains an instance of `Cosntant` and `Expect`. Using [[BoringUtils.bore]], we can connect + * `constant.x` to `expect.y`. * * {{{ * class Top extends Module { @@ -66,11 +61,9 @@ class BoringUtilsException(message: String) extends Exception(message) * * ===Non-hierarchical Boring=== * - * Non-hierarchical boring involves connections from sources to sinks - * that cannot see each other. Here, `x` is described as a source and - * given a name, `uniqueId`, and `y` is described as a sink with the same - * name. This is equivalent to the hierarchical boring example above, but - * requires no modifications to `Top`. + * Non-hierarchical boring involves connections from sources to sinks that cannot see each other. Here, `x` is + * described as a source and given a name, `uniqueId`, and `y` is described as a sink with the same name. This is + * equivalent to the hierarchical boring example above, but requires no modifications to `Top`. * * {{{ * class Constant extends Module { @@ -96,17 +89,13 @@ class BoringUtilsException(message: String) extends Exception(message) * * ==Comments== * - * Both hierarchical and non-hierarchical boring emit FIRRTL annotations - * that describe sources and sinks. These are matched by a `name` key - * that indicates they should be wired together. Hierarhical boring - * safely generates this name automatically. Non-hierarchical boring - * unsafely relies on user input to generate this name. Use of - * non-hierarchical naming may result in naming conflicts that the user - * must handle. + * Both hierarchical and non-hierarchical boring emit FIRRTL annotations that describe sources and sinks. These are + * matched by a `name` key that indicates they should be wired together. Hierarhical boring safely generates this name + * automatically. Non-hierarchical boring unsafely relies on user input to generate this name. Use of non-hierarchical + * naming may result in naming conflicts that the user must handle. * - * The automatic generation of hierarchical names relies on a global, - * mutable namespace. This is currently persistent across circuit - * elaborations. + * The automatic generation of hierarchical names relies on a global, mutable namespace. This is currently persistent + * across circuit elaborations. */ object BoringUtils { /* A global namespace for boring ids */ @@ -125,22 +114,24 @@ object BoringUtils { private def checkName(value: String): Boolean = namespace.get.contains(value) /** Add a named source cross module reference - * * @param component source circuit component * @param name unique identifier for this source - * @param dedup enable dedupblication of modules (necessary if other - * instances exist in the design that are not sources) - * @param uniqueName if true, this will use a non-conflicting name from - * the global namespace + * @param disableDedup disable dedupblication of this source component (this should be true if you are trying to wire + * from specific identical sources differently) + * @param uniqueName if true, this will use a non-conflicting name from the global namespace * @return the name used - * @note if a uniqueName is not specified, the returned name may differ - * from the user-provided name + * @note if a uniqueName is not specified, the returned name may differ from the user-provided name */ - def addSource(component: NamedComponent, name: String, dedup: Boolean = false, uniqueName: Boolean = false): String = { + def addSource( + component: NamedComponent, + name: String, + disableDedup: Boolean = false, + uniqueName: Boolean = false): String = { + val id = if (uniqueName) { newName(name) } else { name } val maybeDedup = - if (dedup) { Seq(new ChiselAnnotation { def toFirrtl = NoDedupAnnotation(component.toNamed.module) }) } - else { Seq[ChiselAnnotation]() } + if (disableDedup) { Seq(new ChiselAnnotation { def toFirrtl = NoDedupAnnotation(component.toNamed.module) }) } + else { Seq[ChiselAnnotation]() } val annotations = Seq(new ChiselAnnotation with RunFirrtlTransform { def toFirrtl = SourceAnnotation(component.toNamed, id) @@ -151,19 +142,20 @@ object BoringUtils { id } - /** Add a named sink cross module reference. Multiple sinks may map to - * the same source. - * + /** Add a named sink cross module reference. Multiple sinks may map to the same source. * @param component sink circuit component * @param name unique identifier for this sink that must resolve to - * @param dedup enable deduplication on sink component (necessary if - * other instances exist in the design that are not sinks) a source - * identifier - * @param forceExists if true, require that the provided `name` paramater - * already exists in the global namespace - * @throws BoringUtilsException if name is expected to exist and it doesn't + * @param disableDedup disable deduplication of this sink component (this should be true if you are trying to wire + * specific, identical sinks differently) + * @param forceExists if true, require that the provided `name` paramater already exists in the global namespace + * @throws BoringUtilsException if name is expected to exist and itdoesn't */ - def addSink(component: InstanceId, name: String, dedup: Boolean = false, forceExists: Boolean = false): Unit = { + def addSink( + component: InstanceId, + name: String, + disableDedup: Boolean = false, + forceExists: Boolean = false): Unit = { + if (forceExists && !checkName(name)) { throw new BoringUtilsException(s"Sink ID '$name' not found in BoringUtils ID namespace") } def moduleName = component.toNamed match { @@ -172,8 +164,8 @@ object BoringUtils { case _ => throw new ChiselException("Can only add a Module or Component sink", null) } val maybeDedup = - if (dedup) { Seq(new ChiselAnnotation { def toFirrtl = NoDedupAnnotation(moduleName) }) } - else { Seq[ChiselAnnotation]() } + if (disableDedup) { Seq(new ChiselAnnotation { def toFirrtl = NoDedupAnnotation(moduleName) }) } + else { Seq[ChiselAnnotation]() } val annotations = Seq(new ChiselAnnotation with RunFirrtlTransform { def toFirrtl = SinkAnnotation(component.toNamed, name) @@ -182,7 +174,6 @@ object BoringUtils { } /** Connect a source to one or more sinks - * * @param source a source component * @param sinks one or more sink components * @return the name of the signal used to connect the source to the -- cgit v1.2.3