From 9ab58c1efb503aec00c22dbd70434038ec87dcab Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Tue, 14 Apr 2020 14:27:07 -0700 Subject: Update RightShiftTests.fir to avoid buggy Counter pattern * See freechipsproject/chisel3#1408 --- test/integration/RightShiftTester.fir | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/RightShiftTester.fir b/test/integration/RightShiftTester.fir index f15a1239..d85757b8 100644 --- a/test/integration/RightShiftTester.fir +++ b/test/integration/RightShiftTester.fir @@ -37,8 +37,8 @@ circuit RightShiftTester : dut.clock <= clock dut.reset <= reset reg T_6 : UInt<2>, clock with : (reset => (reset, UInt<2>("h00"))) + node T_8 = eq(T_6, UInt<2>("h03")) when UInt<1>("h01") : - node T_8 = eq(T_6, UInt<2>("h03")) node T_10 = and(UInt<1>("h00"), T_8) node T_13 = add(T_6, UInt<1>("h01")) node T_14 = tail(T_13, 1) -- cgit v1.2.3 From 7537bb3c18a90ee42ee367bddd90e7d90bd6c90b Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Tue, 14 Apr 2020 14:05:52 -0700 Subject: Honor block scoping of Conditionally in CheckHighForm * Fixes #1505 --- src/main/scala/firrtl/passes/CheckHighForm.scala | 44 +++++++++++++++++------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/src/main/scala/firrtl/passes/CheckHighForm.scala b/src/main/scala/firrtl/passes/CheckHighForm.scala index 889cdae2..5aba26ae 100644 --- a/src/main/scala/firrtl/passes/CheckHighForm.scala +++ b/src/main/scala/firrtl/passes/CheckHighForm.scala @@ -12,6 +12,21 @@ import firrtl.options.Dependency trait CheckHighFormLike { this: Pass => type NameSet = collection.mutable.HashSet[String] + private object ScopeView { + def apply(): ScopeView = new ScopeView(new NameSet, List(new NameSet)) + } + + private class ScopeView private (moduleNS: NameSet, scopes: List[NameSet]) { + require(scopes.nonEmpty) + def declare(name: String): Unit = { + moduleNS += name + scopes.head += name + } + def legalDecl(name: String): Boolean = !moduleNS.contains(name) + def legalRef(name: String): Boolean = scopes.exists(_.contains(name)) + def childScope(): ScopeView = new ScopeView(moduleNS, new NameSet +: scopes) + } + // Custom Exceptions class NotUniqueException(info: Info, mname: String, name: String) extends PassException( s"$info: [module $mname] Reference $name does not have a unique name.") @@ -176,11 +191,11 @@ trait CheckHighFormLike { this: Pass => } } - def checkHighFormE(info: Info, mname: String, names: NameSet)(e: Expression): Unit = { + def checkHighFormE(info: Info, mname: String, names: ScopeView)(e: Expression): Unit = { e match { - case ex: Reference if !names(ex.name) => + case ex: Reference if !names.legalRef(ex.name) => errors.append(new UndeclaredReferenceException(info, mname, ex.name)) - case ex: WRef if !names(ex.name) => + case ex: WRef if !names.legalRef(ex.name) => errors.append(new UndeclaredReferenceException(info, mname, ex.name)) case ex: UIntLiteral if ex.value < 0 => errors.append(new NegUIntException(info, mname)) @@ -194,10 +209,10 @@ trait CheckHighFormLike { this: Pass => e foreach checkHighFormE(info, mname, names) } - def checkName(info: Info, mname: String, names: NameSet)(name: String): Unit = { - if (names(name)) + def checkName(info: Info, mname: String, names: ScopeView)(name: String): Unit = { + if (!names.legalDecl(name)) errors.append(new NotUniqueException(info, mname, name)) - names += name + names.declare(name) } def checkInstance(info: Info, child: String, parent: String): Unit = { @@ -209,7 +224,7 @@ trait CheckHighFormLike { this: Pass => errors.append(new InstanceLoop(info, parent, childToParent mkString "->")) } - def checkHighFormS(minfo: Info, mname: String, names: NameSet)(s: Statement): Unit = { + def checkHighFormS(minfo: Info, mname: String, names: ScopeView)(s: Statement): Unit = { val info = get_info(s) match {case NoInfo => minfo case x => x} s foreach checkName(info, mname, names) s match { @@ -233,13 +248,18 @@ trait CheckHighFormLike { this: Pass => } s foreach checkHighFormT(info, mname) s foreach checkHighFormE(info, mname, names) - s foreach checkHighFormS(minfo, mname, names) + s match { + case Conditionally(_,_, conseq, alt) => + checkHighFormS(minfo, mname, names.childScope())(conseq) + checkHighFormS(minfo, mname, names.childScope())(alt) + case _ => s foreach checkHighFormS(minfo, mname, names) + } } - def checkHighFormP(mname: String, names: NameSet)(p: Port): Unit = { - if (names(p.name)) + def checkHighFormP(mname: String, names: ScopeView)(p: Port): Unit = { + if (!names.legalDecl(p.name)) errors.append(new NotUniqueException(NoInfo, mname, p.name)) - names += p.name + names.declare(p.name) checkHighFormT(p.info, mname)(p.tpe) } @@ -255,7 +275,7 @@ trait CheckHighFormLike { this: Pass => } def checkHighFormM(m: DefModule): Unit = { - val names = new NameSet + val names = ScopeView() m foreach checkHighFormP(m.name, names) m foreach checkHighFormS(m.info, m.name, names) m match { -- cgit v1.2.3 From 3c1a8e88844f2df41a60f5a8902b61ccf67b1382 Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Tue, 14 Apr 2020 14:06:53 -0700 Subject: Add Conditionally scoping tests to CheckSpec * Add specific test for shadowing --- src/test/scala/firrtlTests/CheckSpec.scala | 47 ++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/test/scala/firrtlTests/CheckSpec.scala b/src/test/scala/firrtlTests/CheckSpec.scala index c622bde5..b49054ad 100644 --- a/src/test/scala/firrtlTests/CheckSpec.scala +++ b/src/test/scala/firrtlTests/CheckSpec.scala @@ -352,6 +352,53 @@ class CheckSpec extends AnyFlatSpec with Matchers { } } + "Conditionally statements" should "create a new scope" in { + val input = + s"""|circuit scopes: + | module scopes: + | input i: UInt<1> + | output o: UInt<1> + | when i: + | node x = not(i) + | o <= and(x, i) + |""".stripMargin + assertThrows[CheckHighForm.UndeclaredReferenceException] { + checkHighInput(input) + } + } + + "Attempting to shadow a component name" should "throw an error" in { + val input = + s"""|circuit scopes: + | module scopes: + | input i: UInt<1> + | output o: UInt<1> + | wire x: UInt<1> + | when i: + | node x = not(i) + | o <= and(x, i) + |""".stripMargin + assertThrows[CheckHighForm.NotUniqueException] { + checkHighInput(input) + } + } + + "Conditionally statements" should "create separate consequent and alternate scopes" in { + val input = + s"""|circuit scopes: + | module scopes: + | input i: UInt<1> + | output o: UInt<1> + | o <= i + | when i: + | node x = not(i) + | else: + | o <= and(x, i) + |""".stripMargin + assertThrows[CheckHighForm.UndeclaredReferenceException] { + checkHighInput(input) + } + } } object CheckSpec { -- cgit v1.2.3 From b373b5a6f1b1c45ed474e3b037afb3ec8ec03d9a Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Tue, 14 Apr 2020 15:14:14 -0700 Subject: Add adapter to make current CHIRRTL mport scoping legal * See #1505 * Inferred mports are implicitly added to scope of their parent mem * This allows current chisel3 emission to work with new scope checks * This may change in a future refactor of CHIRRTL memory ports --- src/main/scala/firrtl/passes/CheckHighForm.scala | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/main/scala/firrtl/passes/CheckHighForm.scala b/src/main/scala/firrtl/passes/CheckHighForm.scala index 5aba26ae..512602cf 100644 --- a/src/main/scala/firrtl/passes/CheckHighForm.scala +++ b/src/main/scala/firrtl/passes/CheckHighForm.scala @@ -22,6 +22,10 @@ trait CheckHighFormLike { this: Pass => moduleNS += name scopes.head += name } + def expandMPortVisibility(port: CDefMPort): Unit = { + // Legacy CHIRRTL ports are visible in any scope where their parent memory is visible + scopes.find(_.contains(port.mem)).getOrElse(scopes.head) += port.name + } def legalDecl(name: String): Boolean = !moduleNS.contains(name) def legalRef(name: String): Boolean = scopes.exists(_.contains(name)) def childScope(): ScopeView = new ScopeView(moduleNS, new NameSet +: scopes) @@ -243,7 +247,10 @@ trait CheckHighFormLike { this: Pass => case sx: Connect => checkValidLoc(info, mname, sx.loc) case sx: PartialConnect => checkValidLoc(info, mname, sx.loc) case sx: Print => checkFstring(info, mname, sx.string, sx.args.length) - case _: CDefMemory | _: CDefMPort => errorOnChirrtl(info, mname, s).foreach { e => errors.append(e) } + case _: CDefMemory => errorOnChirrtl(info, mname, s).foreach { e => errors.append(e) } + case mport: CDefMPort => + errorOnChirrtl(info, mname, s).foreach { e => errors.append(e) } + names.expandMPortVisibility(mport) case sx => // Do Nothing } s foreach checkHighFormT(info, mname) -- cgit v1.2.3 From 1220a23ac9dfc26dbf5475ef064b644cdd1d7983 Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Tue, 14 Apr 2020 15:29:43 -0700 Subject: Fix out-of-scope reference in handwritten CHIRRTL mem test --- src/test/resources/features/ChirrtlMems.fir | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/resources/features/ChirrtlMems.fir b/src/test/resources/features/ChirrtlMems.fir index c51e3b78..de5b3cf3 100644 --- a/src/test/resources/features/ChirrtlMems.fir +++ b/src/test/resources/features/ChirrtlMems.fir @@ -14,13 +14,13 @@ circuit ChirrtlMems : raddr <= add(raddr, UInt(1)) infer mport r = ram[raddr], newClock + reg waddr : UInt<4>, clock with : (reset => (reset, UInt(0))) + waddr <= add(waddr, UInt(1)) + when wen : node newerClock = clock - reg waddr : UInt<4>, clock with : (reset => (reset, UInt(0))) - waddr <= add(waddr, UInt(1)) infer mport w = ram[waddr], newerClock w <= waddr - when eq(waddr, UInt(0)) : raddr <= UInt(0) -- cgit v1.2.3