Error on ExtModules w/ same defname, diff. ports (#1734)

* Use signed output in LargeParamExecutionTest

Change the Verilog used in LargeParamExecutionTest to match its
ExtModule specification. An ExtModule with an SInt port should map to
a separate Verilog module with a signed port and this is disjoint from
an ExtModule with a UInt port.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>

* Error on ExtModules w/ same defname, diff. ports

Adds a high form check to ensure that external modules that have the
same defname also have exactly the same ports.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

5 files changed, 181 insertions, 12 deletions

diff --git a/src/main/scala/firrtl/passes/CheckHighForm.scala b/src/main/scala/firrtl/passes/CheckHighForm.scala
index fb5dd1ca..3ba2a3db 100644
--- a/src/main/scala/firrtl/passes/CheckHighForm.scala
+++ b/src/main/scala/firrtl/passes/CheckHighForm.scala
@@ -54,6 +54,8 @@ trait CheckHighFormLike { this: Pass =>
     s"$info: Repeat definition of module $mname")
   class DefnameConflictException(info: Info, mname: String, defname: String) extends PassException(
     s"$info: defname $defname of extmodule $mname conflicts with an existing module")
+  class DefnameDifferentPortsException(info: Info, mname: String, defname: String) extends PassException(
+    s"""$info: ports of extmodule $mname with defname $defname are different for an extmodule with the same defname""")
   class ModuleNotDefinedException(info: Info, mname: String, name: String) extends PassException(
     s"$info: Module $name is not defined.")
   class IncorrectNumArgsException(info: Info, mname: String, op: String, n: Int) extends PassException(
@@ -100,9 +102,37 @@ trait CheckHighFormLike { this: Pass =>
       m => errors.append(new ModuleNameNotUniqueException(m.info, m.name))
     }
 
+    /** Strip all widths from types */
+    def stripWidth(tpe: Type): Type = tpe match {
+      case a: GroundType    => a.mapWidth(_ => UnknownWidth)
+      case a: AggregateType => a.mapType(stripWidth)
+    }
+
+    val extmoduleCollidingPorts = c.modules.collect {
+      case a: ExtModule => a
+    }.groupBy(a => (a.defname, a.params.nonEmpty)).map {
+      /* There are no parameters, so all ports must match exactly. */
+      case (k@ (_, false), a) =>
+        k -> a.map(_.copy(info=NoInfo)).map(_.ports.map(_.copy(info=NoInfo))).toSet
+      /* If there are parameters, then only port names must match because parameters could parameterize widths.
+       * This means that this check cannot produce false positives, but can have false negatives.
+       */
+      case (k@ (_, true),  a) =>
+        k -> a.map(_.copy(info=NoInfo)).map(_.ports.map(_.copy(info=NoInfo).mapType(stripWidth))).toSet
+    }.filter(_._2.size > 1)
+
     c.modules.collect {
-      case ExtModule(info, name, _, defname, _) if (intModuleNames.contains(defname)) =>
-        errors.append(new DefnameConflictException(info, name, defname))
+      case a: ExtModule =>
+        a match {
+          case ExtModule(info, name, _, defname, _) if (intModuleNames.contains(defname)) =>
+            errors.append(new DefnameConflictException(info, name, defname))
+          case _ =>
+        }
+        a match {
+          case ExtModule(info, name, _, defname, params) if extmoduleCollidingPorts.contains((defname, params.nonEmpty)) =>
+            errors.append(new DefnameDifferentPortsException(info, name, defname))
+          case _ =>
+        }
     }
 
     def checkHighFormPrimop(info: Info, mname: String, e: DoPrim): Unit = {
diff --git a/src/test/resources/blackboxes/LargeParam.v b/src/test/resources/blackboxes/LargeParam.v
index 84e1a1cf..3a0dcd9e 100644
--- a/src/test/resources/blackboxes/LargeParam.v
+++ b/src/test/resources/blackboxes/LargeParam.v
@@ -1,7 +1,12 @@
 // See LICENSE for license details.
-module LargeParam #(parameter DATA=0, WIDTH=1) (
+module LargeParamUnsigned #(parameter DATA=0, WIDTH=1) (
   output [WIDTH-1:0] out
 );
   assign out = DATA;
 endmodule
 
+module LargeParamSigned #(parameter DATA=0, WIDTH=1) (
+  output signed [WIDTH-1:0] out
+);
+  assign out = DATA;
+endmodule
diff --git a/src/test/resources/blackboxes/LargeParamTester.fir b/src/test/resources/blackboxes/LargeParamTester.fir
index bb0ebdf5..29027c36 100644
--- a/src/test/resources/blackboxes/LargeParamTester.fir
+++ b/src/test/resources/blackboxes/LargeParamTester.fir
@@ -3,21 +3,21 @@ circuit LargeParamTester :
   extmodule LargeParam :
     output out : UInt<1>
 
-    defname = LargeParam
+    defname = LargeParamUnsigned
     parameter WIDTH = 1
     parameter DATA = 0
 
   extmodule LargeParam_1 :
     output out : UInt<128>
 
-    defname = LargeParam
+    defname = LargeParamUnsigned
     parameter WIDTH = 128
     parameter DATA = 9223372036854775807000
 
   extmodule LargeParam_2 :
     output out : SInt<128>
 
-    defname = LargeParam
+    defname = LargeParamSigned
     parameter WIDTH = 128
     parameter DATA = -9223372036854775807000
 
@@ -40,4 +40,3 @@ circuit LargeParamTester :
         printf(clock, UInt(1), "Assertion failed\nTest Failed!\n")
         stop(clock, UInt(1), 1)
       stop(clock, UInt(1), 0)
-
diff --git a/src/test/scala/firrtlTests/CheckSpec.scala b/src/test/scala/firrtlTests/CheckSpec.scala
index b49054ad..5c38bf30 100644
--- a/src/test/scala/firrtlTests/CheckSpec.scala
+++ b/src/test/scala/firrtlTests/CheckSpec.scala
@@ -399,6 +399,142 @@ class CheckSpec extends AnyFlatSpec with Matchers {
       checkHighInput(input)
     }
   }
+
+  behavior of "CheckHighForm running on circuits containing ExtModules"
+
+  it should "throw an exception if parameterless ExtModules have the same ports, but different widths" in {
+    val input =
+      s"""|circuit Foo:
+          |  extmodule Bar:
+          |    input a: UInt<1>
+          |    defname = bar
+          |  extmodule Baz:
+          |    input a: UInt<2>
+          |    defname = bar
+          |  module Foo:
+          |    skip
+          |""".stripMargin
+    assertThrows[CheckHighForm.DefnameDifferentPortsException] {
+      try {
+        checkHighInput(input)
+      } catch {
+        case e: firrtl.passes.PassExceptions => throw e.exceptions.head
+      }
+    }
+  }
+
+  it should "throw an exception if ExtModules have different port names, but identical widths" in {
+    val input =
+      s"""|circuit Foo:
+          |  extmodule Bar:
+          |    input a: UInt<1>
+          |    defname = bar
+          |  extmodule Baz:
+          |    input b: UInt<1>
+          |    defname = bar
+          |  module Foo:
+          |    skip
+          |""".stripMargin
+    assertThrows[CheckHighForm.DefnameDifferentPortsException] {
+      try {
+        checkHighInput(input)
+      } catch {
+        case e: firrtl.passes.PassExceptions => throw e.exceptions.head
+      }
+    }
+  }
+
+  it should "NOT throw an exception if ExtModules have parameters, matching port names, but different widths" in {
+    val input =
+      s"""|circuit Foo:
+          |  extmodule Bar:
+          |    input a: UInt<1>
+          |    defname = bar
+          |    parameter width = 1
+          |  extmodule Baz:
+          |    input a: UInt<2>
+          |    defname = bar
+          |    parameter width = 2
+          |  module Foo:
+          |    skip
+          |""".stripMargin
+    checkHighInput(input)
+  }
+
+  it should "throw an exception if ExtModules have matching port names and widths, but a different order" in {
+    val input =
+      s"""|circuit Foo:
+          |  extmodule Bar:
+          |    input a: UInt<1>
+          |    input b: UInt<1>
+          |    defname = bar
+          |  extmodule Baz:
+          |    input b: UInt<1>
+          |    input a: UInt<1>
+          |    defname = bar
+          |  module Foo:
+          |    skip
+          |""".stripMargin
+    assertThrows[CheckHighForm.DefnameDifferentPortsException] {
+      try {
+        checkHighInput(input)
+      } catch {
+        case e: firrtl.passes.PassExceptions => throw e.exceptions.head
+      }
+    }
+  }
+
+  it should "throw an exception if ExtModules have matching port names, but one is a Clock and one is a UInt<1>" in {
+    val input =
+      s"""|circuit Foo:
+          |  extmodule Bar:
+          |    input a: UInt<1>
+          |    defname = bar
+          |  extmodule Baz:
+          |    input a: Clock
+          |    defname = bar
+          |  module Foo:
+          |    skip
+          |""".stripMargin
+    assertThrows[CheckHighForm.DefnameDifferentPortsException] {
+      try {
+        checkHighInput(input)
+      } catch {
+        case e: firrtl.passes.PassExceptions => throw e.exceptions.head
+      }
+    }
+  }
+
+  it should "throw an exception if ExtModules have differing concrete reset types" in {
+    def input(rst1: String, rst2: String) =
+      s"""|circuit Foo:
+          |  extmodule Bar:
+          |    input rst: $rst1
+          |    defname = bar
+          |  extmodule Baz:
+          |    input rst: $rst2
+          |    defname = bar
+          |  module Foo:
+          |    skip
+          |""".stripMargin
+    info("exception thrown for 'UInt<1>' compared to 'AsyncReset'")
+    assertThrows[CheckHighForm.DefnameDifferentPortsException] {
+      try {
+        checkHighInput(input("UInt<1>", "AsyncReset"))
+      } catch {
+        case e: firrtl.passes.PassExceptions => throw e.exceptions.head
+      }
+    }
+    info("exception thrown for 'UInt<1>' compared to 'Reset'")
+    assertThrows[CheckHighForm.DefnameDifferentPortsException] {
+      try {
+        checkHighInput(input("UInt<1>", "Reset"))
+      } catch {
+        case e: firrtl.passes.PassExceptions => throw e.exceptions.head
+      }
+    }
+  }
+
 }
 
 object CheckSpec {
diff --git a/src/test/scala/firrtlTests/transforms/DedupTests.scala b/src/test/scala/firrtlTests/transforms/DedupTests.scala
index 5776db31..8ab3026c 100644
--- a/src/test/scala/firrtlTests/transforms/DedupTests.scala
+++ b/src/test/scala/firrtlTests/transforms/DedupTests.scala
@@ -193,13 +193,13 @@ class DedupModuleTests extends HighTransformSpec {
           |  module A_ : @[xx 1:1]
           |    output y: UInt<1> @[xx 1:1]
           |    inst c of C
-          |    y <= c.v
+          |    y <= c.u
           |  extmodule B : @[aa 3:3]
           |    output u : UInt<1> @[aa 4:4]
           |    defname = BB
           |    parameter N = 0
           |  extmodule C : @[bb 5:5]
-          |    output v : UInt<1> @[bb 6:6]
+          |    output u : UInt<1> @[bb 6:6]
           |    defname = BB
           |    parameter N = 0
         """.stripMargin
@@ -238,13 +238,13 @@ class DedupModuleTests extends HighTransformSpec {
           |  module A_ : @[xx 1:1]
           |    output y: UInt<1> @[xx 1:1]
           |    inst c of C
-          |    y <= c.v
+          |    y <= c.u
           |  extmodule B : @[aa 3:3]
           |    output u : UInt<1> @[aa 4:4]
           |    defname = ${defnames._1}
           |    parameter N = ${params._1}
           |  extmodule C : @[bb 5:5]
-          |    output v : UInt<1> @[bb 6:6]
+          |    output u : UInt<1> @[bb 6:6]
           |    defname = ${defnames._2}
           |    parameter N = ${params._2}
         """.stripMargin
@@ -875,4 +875,3 @@ class DedupModuleTests extends HighTransformSpec {
     csDeduped.annotations.toSeq should contain (expectedAnnB)
   }
 }
-