From 1f63318b849012ba5655ac26774db383cf57f37d Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Thu, 26 Dec 2019 13:27:39 -0800 Subject: Respect last connect semantics in InferResets InferResets will now support last connect semantics (within the same scope) when determining the concrete reset type for components of type Reset. This only includes *unconditional* last connects; it remains illegal to drive a component of type Reset with different concrete types under differing when conditions. For example, the following is now legal: input a : UInt<1> input b : AsyncReset output z : Reset z <= a z <= b The second connect will when and z will be of type AsyncReset. The following remains illegal: input a : UInt<1> input b : AsyncReset input c : UInt<1> output z : Reset z <= a when c : z <= b This commit also ensures that components of type Reset with no drivers (or only invalidation) default to type UInt<1>. This fixes a bug where the transform would crash with such input. --- src/main/scala/firrtl/transforms/InferResets.scala | 49 +++++++---- src/test/scala/firrtlTests/InferResetsSpec.scala | 97 +++++++++++++++++----- 2 files changed, 112 insertions(+), 34 deletions(-) (limited to 'src') diff --git a/src/main/scala/firrtl/transforms/InferResets.scala b/src/main/scala/firrtl/transforms/InferResets.scala index 70e2b76c..ea5b3092 100644 --- a/src/main/scala/firrtl/transforms/InferResets.scala +++ b/src/main/scala/firrtl/transforms/InferResets.scala @@ -36,7 +36,11 @@ object InferResets { private case class TypeDriver(tpe: Type, target: () => ReferenceTarget) extends ResetDriver { override def toString: String = s"TypeDriver(${tpe.serialize}, $target)" } - + // When a [[ResetType]] is invalidated, we record the InvalidDrive + // If there are no types but invalid drivers, we default to BoolType + private case object InvalidDriver extends ResetDriver { + def defaultType: Type = Utils.BoolType + } // Type hierarchy representing the path to a leaf type in an aggregate type structure // Used by this [[InferResets]] to pinpoint instances of [[ResetType]] and their inferred type @@ -84,7 +88,7 @@ class InferResets extends Transform { // Collect all drivers for circuit elements of type ResetType private def analyze(c: Circuit): Map[ReferenceTarget, List[ResetDriver]] = { - type DriverMap = mutable.HashMap[ReferenceTarget, mutable.ListBuffer[ResetDriver]] + type DriverMap = mutable.HashMap[ReferenceTarget, List[ResetDriver]] def onMod(mod: DefModule): DriverMap = { val instMap = mutable.Map[String, String]() // We need to convert submodule port targets into targets on the Module port itself @@ -116,7 +120,7 @@ class InferResets extends Transform { case ResetType => TargetDriver(makeTarget(exp)) case tpe => TypeDriver(tpe, () => makeTarget(exp)) } - map.getOrElseUpdate(target, mutable.ListBuffer()) += driver + map(target) = driver :: Nil } } stmt match { @@ -136,6 +140,16 @@ class InferResets extends Transform { for ((i, j) <- points) { markResetDriver(locs(i), exps(j)) } + case IsInvalid(_, lhs) => + val exprs = Utils.create_exps(lhs) + for (expr <- exprs) { + // Ignore leaves that are not of type ResetType + // Unlike in markResetDriver, flow is irrelevant for invalidation + if (expr.tpe == ResetType) { + val target = makeTarget(expr) + map(target) = InvalidDriver :: Nil + } + } case WDefInstance(_, inst, module, _) => instMap += (inst -> module) case Conditionally(_, _, con, alt) => @@ -143,12 +157,12 @@ class InferResets extends Transform { val altMap = new DriverMap onStmt(conMap)(con) onStmt(altMap)(alt) - // Default to outerscope if not found in alt + // Default to outerscope if not found on either side + val conLookup = conMap.orElse(map).lift val altLookup = altMap.orElse(map).lift for (key <- conMap.keys ++ altMap.keys) { - val ds = map.getOrElseUpdate(key, mutable.ListBuffer()) - conMap.get(key).foreach(ds ++= _) - altLookup(key).foreach(ds ++= _) + val values = conLookup(key).getOrElse(Nil) ++ altLookup(key).getOrElse(Nil) + map(key) = values } case other => other.foreach(onStmt(map)) } @@ -167,17 +181,22 @@ class InferResets extends Transform { val res = mutable.Map[ReferenceTarget, Type]() val errors = new Errors def rec(target: ReferenceTarget): Type = { - val drivers = map(target) + val drivers = map.getOrElse(target, Nil) res.getOrElseUpdate(target, { - val tpes = drivers.map { - case TargetDriver(t) => TypeDriver(rec(t), () => t) - case td: TypeDriver => td + val tpes = drivers.flatMap { + case TargetDriver(t) => Some(TypeDriver(rec(t), () => t)) + case td: TypeDriver => Some(td) + case InvalidDriver => None }.groupBy(_.tpe) - if (tpes.keys.size != 1) { - // Multiple types of driver! - errors.append(DifferingDriverTypesException(target, tpes.toSeq)) + tpes.keys.size match { + // This can occur if something of type Reset has no driver + case 0 => InvalidDriver.defaultType + case 1 => tpes.keys.head + case _ => + // Multiple types of driver! + errors.append(DifferingDriverTypesException(target, tpes.toSeq)) + tpes.keys.head } - tpes.keys.head }) } for ((target, _) <- map) { diff --git a/src/test/scala/firrtlTests/InferResetsSpec.scala b/src/test/scala/firrtlTests/InferResetsSpec.scala index ac13033a..501dce20 100644 --- a/src/test/scala/firrtlTests/InferResetsSpec.scala +++ b/src/test/scala/firrtlTests/InferResetsSpec.scala @@ -4,7 +4,7 @@ package firrtlTests import firrtl._ import firrtl.ir._ -import firrtl.passes.{CheckHighForm, CheckTypes} +import firrtl.passes.{CheckHighForm, CheckTypes, CheckInitialization} import firrtl.transforms.InferResets import FirrtlCheckers._ @@ -37,7 +37,6 @@ class InferResetsSpec extends FirrtlFlatSpec { | y <= asFixedPoint(r, 0) | z <= asAsyncReset(r)""".stripMargin ) - println(result.getEmittedCircuit) result should containLine ("wire r : UInt<1>") result should containLine ("r <= a") result should containLine ("v <= asUInt(r)") @@ -125,44 +124,68 @@ class InferResetsSpec extends FirrtlFlatSpec { result should containTree { case Port(_, "buzz_bar_1_b", Input, AsyncResetType) => true } } - it should "NOT allow last connect semantics to pick the right type for Reset" in { - an [InferResets.DifferingDriverTypesException] shouldBe thrownBy { + it should "not crash if a ResetType has no drivers" in { + a [CheckInitialization.RefNotInitializedException] shouldBe thrownBy { + compile(s""" + |circuit test : + | module test : + | output out : Reset + | wire w : Reset + | out <= w + | out <= UInt(1) + |""".stripMargin + ) + } + } + + it should "allow last connect semantics to pick the right type for Reset" in { + val result = compile(s""" |circuit top : | module top : | input reset0 : AsyncReset | input reset1 : UInt<1> | output out : Reset + | wire w0 : Reset | wire w1 : Reset - | wire w2 : Reset - | w1 <= reset0 - | w2 <= reset1 + | w0 <= reset0 + | w1 <= reset1 + | out <= w0 | out <= w1 - | out <= w2 |""".stripMargin ) - } + result should containTree { case DefWire(_, "w0", AsyncResetType) => true } + result should containTree { case DefWire(_, "w1", BoolType) => true } + result should containTree { case Port(_, "out", Output, BoolType) => true } } - it should "NOT support last connect semantics across whens" in { - an [InferResets.DifferingDriverTypesException] shouldBe thrownBy { + it should "support last connect semantics across whens" in { + val result = compile(s""" |circuit top : | module top : | input reset0 : AsyncReset - | input reset1 : UInt<1> - | input en0 : UInt<1> + | input reset1 : AsyncReset + | input reset2 : UInt<1> + | input en : UInt<1> | output out : Reset + | wire w0 : Reset | wire w1 : Reset | wire w2 : Reset - | w1 <= reset0 - | w2 <= reset1 - | out <= w1 - | when en0 : - | out <= w2 + | w0 <= reset0 + | w1 <= reset1 + | w2 <= reset2 + | out <= w2 + | when en : + | out <= w0 + | else : + | out <= w1 |""".stripMargin ) - } + result should containTree { case DefWire(_, "w0", AsyncResetType) => true } + result should containTree { case DefWire(_, "w1", AsyncResetType) => true } + result should containTree { case DefWire(_, "w2", BoolType) => true } + result should containTree { case Port(_, "out", Output, AsyncResetType) => true } } it should "not allow different Reset Types to drive a single Reset" in { @@ -186,6 +209,42 @@ class InferResetsSpec extends FirrtlFlatSpec { } } + it should "allow concrete reset types to overrule invalidation" in { + val result = compile(s""" + |circuit test : + | module test : + | input in : AsyncReset + | output out : Reset + | out is invalid + | out <= in + |""".stripMargin) + result should containTree { case Port(_, "out", Output, AsyncResetType) => true } + } + + it should "default to BoolType for Resets that are only invalidated" in { + val result = compile(s""" + |circuit test : + | module test : + | output out : Reset + | out is invalid + |""".stripMargin) + result should containTree { case Port(_, "out", Output, BoolType) => true } + } + + it should "not error if component of ResetType is invalidated and connected to an AsyncResetType" in { + val result = compile(s""" + |circuit test : + | module test : + | input cond : UInt<1> + | input in : AsyncReset + | output out : Reset + | out is invalid + | when cond : + | out <= in + |""".stripMargin) + result should containTree { case Port(_, "out", Output, AsyncResetType) => true } + } + it should "allow ResetType to drive AsyncResets or UInt<1>" in { val result1 = compile(s""" |circuit top : -- cgit v1.2.3 From 4fd2e6e98917f1d6424453b878f357756840a819 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Fri, 27 Dec 2019 08:30:14 -0800 Subject: Minor code cleansup in InferResets * Move Map lookup into closure so it only occurs if necessary * Replace gender with flow and improve code clarity --- src/main/scala/firrtl/transforms/InferResets.scala | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/main/scala/firrtl/transforms/InferResets.scala b/src/main/scala/firrtl/transforms/InferResets.scala index ea5b3092..fbc915e2 100644 --- a/src/main/scala/firrtl/transforms/InferResets.scala +++ b/src/main/scala/firrtl/transforms/InferResets.scala @@ -108,19 +108,18 @@ class InferResets extends Transform { def onStmt(map: DriverMap)(stmt: Statement): Unit = { // Mark driver of a ResetType leaf def markResetDriver(lhs: Expression, rhs: Expression): Unit = { - val lflip = Utils.to_flip(Utils.gender(lhs)) - if ((lflip == Default && lhs.tpe == ResetType) || - (lflip == Flip && rhs.tpe == ResetType)) { - val (loc, exp) = lflip match { - case Default => (lhs, rhs) - case Flip => (rhs, lhs) - } - val target = makeTarget(loc) + val con = Utils.flow(lhs) match { + case SinkFlow if lhs.tpe == ResetType => Some((lhs, rhs)) + case SourceFlow if rhs.tpe == ResetType => Some((rhs, lhs)) + // If sink is not ResetType, do nothing + case _ => None + } + con.foreach { case (loc, exp) => val driver = exp.tpe match { case ResetType => TargetDriver(makeTarget(exp)) case tpe => TypeDriver(tpe, () => makeTarget(exp)) } - map(target) = driver :: Nil + map(makeTarget(loc)) = driver :: Nil } } stmt match { @@ -181,8 +180,8 @@ class InferResets extends Transform { val res = mutable.Map[ReferenceTarget, Type]() val errors = new Errors def rec(target: ReferenceTarget): Type = { - val drivers = map.getOrElse(target, Nil) res.getOrElseUpdate(target, { + val drivers = map.getOrElse(target, Nil) val tpes = drivers.flatMap { case TargetDriver(t) => Some(TypeDriver(rec(t), () => t)) case td: TypeDriver => Some(td) -- cgit v1.2.3