[FIRRTL] Verify main module is a module, and is public. (#7439)

Add tests for new errors and existing error (changed). Op errors should remember to make sense when read prefixed with "mydialect.opname op ".
llvm · Aug 6, 2024 · de281de · de281de
1 parent fd2b37a
commit de281de
Show file tree

Hide file tree

Showing 6 changed files with 38 additions and 14 deletions.
diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp
@@ -519,8 +519,16 @@ LogicalResult CircuitOp::verifyRegions() {
 
   mlir::SymbolTable symtbl(getOperation());
 
-  if (!symtbl.lookup(main))
-    return emitOpError().append("Module with same name as circuit not found");
+  auto *mainModule = symtbl.lookup(main);
+  if (!mainModule)
+    return emitOpError().append(
+        "does not contain module with same name as circuit");
+  if (!isa<FModuleLike>(mainModule))
+    return mainModule->emitError(
+        "entity with name of circuit must be a module");
+  if (symtbl.getSymbolVisibility(mainModule) !=
+      mlir::SymbolTable::Visibility::Public)
+    return mainModule->emitError("main module must be public");
 
   // Store a mapping of defname to either the first external module
   // that defines it or, preferentially, the first external module

diff --git a/test/Dialect/FIRRTL/annotations-errors.mlir b/test/Dialect/FIRRTL/annotations-errors.mlir
@@ -438,23 +438,19 @@ firrtl.circuit "Top" attributes {
 // -----
 // OutputDirAnnotation targeting a non-public module should fail.
 
-// expected-error @below {{Unable to apply annotation: {class = "circt.OutputDirAnnotation", dirname = "foo", target = "~Top|Top"}}}
+// expected-error @below {{Unable to apply annotation: {class = "circt.OutputDirAnnotation", dirname = "foo", target = "~Top|NotTop"}}}
 firrtl.circuit "Top" attributes {
   rawAnnotations = [
     {
       class = "circt.OutputDirAnnotation",
       dirname = "foo",
-      target = "~Top|Top"
-    },
-        {
-      class = "circt.OutputDirAnnotation",
-      dirname = "foo",
-      target = "~Top|Top"
+      target = "~Top|NotTop"
     }
   ]
 } {
+  firrtl.module @Top() {}
   // expected-error @below {{circt.OutputDirAnnotation must target a public module}}
-  firrtl.module private @Top() {}
+  firrtl.module private @NotTop() {}
 }
 
 // -----

diff --git a/test/Dialect/FIRRTL/errors.mlir b/test/Dialect/FIRRTL/errors.mlir
@@ -2554,3 +2554,23 @@ firrtl.module @LHSTypes() {
 }
 }
 
+// -----
+
+// expected-error @below {{op does not contain module with same name as circuit}}
+firrtl.circuit "NoMain" { }
+
+// -----
+
+firrtl.circuit "PrivateMain" {
+  // expected-error @below {{main module must be public}}
+  firrtl.module private @PrivateMain() {}
+}
+
+// -----
+
+firrtl.circuit "MainNotModule" {
+  // expected-error @below {{entity with name of circuit must be a module}}
+  func.func @MainNotModule() {
+    return
+  }
+}
diff --git a/test/Dialect/FIRRTL/lower-classes.mlir b/test/Dialect/FIRRTL/lower-classes.mlir
@@ -335,7 +335,7 @@ firrtl.circuit "AnyCast" {
 // CHECK-LABEL: firrtl.circuit "ModuleWithPropertySubmodule"
 firrtl.circuit "ModuleWithPropertySubmodule" {
   // CHECK: om.class @ModuleWithPropertySubmodule_Class
-  firrtl.module private @ModuleWithPropertySubmodule() {
+  firrtl.module @ModuleWithPropertySubmodule() {
     %c0 = firrtl.integer 0
     // CHECK: om.object @SubmoduleWithProperty_Class
     %inst.prop = firrtl.instance inst @SubmoduleWithProperty(in prop: !firrtl.integer)

diff --git a/test/Dialect/FIRRTL/lower-intmodules.mlir b/test/Dialect/FIRRTL/lower-intmodules.mlir
@@ -49,8 +49,8 @@ firrtl.circuit "ProbeIntrinsicTest" {
   // CHECK-NOT: firrtl.intmodule private @FPGAProbeIntrinsic
   firrtl.intmodule private @FPGAProbeIntrinsic(in data: !firrtl.uint, in clock: !firrtl.clock) attributes {intrinsic = "circt_fpga_probe"}
 
-  // CHECK-LABEL: firrtl.module private @ProbeIntrinsicTest
-  firrtl.module private @ProbeIntrinsicTest(in %clock : !firrtl.clock, in %data : !firrtl.uint<32>) {
+  // CHECK-LABEL: firrtl.module @ProbeIntrinsicTest
+  firrtl.module @ProbeIntrinsicTest(in %clock : !firrtl.clock, in %data : !firrtl.uint<32>) {
     // CHECK:      [[DATA:%.+]] = firrtl.wire : !firrtl.uint
     // CHECK-NEXT: [[CLOCK:%.+]] = firrtl.wire : !firrtl.clock
     // CHECK-NEXT: firrtl.int.generic "circt_fpga_probe" [[DATA]], [[CLOCK]] : (!firrtl.uint, !firrtl.clock) -> ()

diff --git a/test/Dialect/FIRRTL/lower-intrinsics-errors.mlir b/test/Dialect/FIRRTL/lower-intrinsics-errors.mlir
@@ -1,7 +1,7 @@
 // RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(firrtl.module(firrtl-lower-intrinsics)))' -verify-diagnostics --split-input-file %s
 
 firrtl.circuit "UnknownIntrinsic" {
-  firrtl.module private @UnknownIntrinsic(in %data: !firrtl.uint<32>) {
+  firrtl.module @UnknownIntrinsic(in %data: !firrtl.uint<32>) {
     %0 = firrtl.wire : !firrtl.uint<32>
     // expected-error @below {{unknown intrinsic}}
     // expected-error @below {{failed to legalize}}