[FIRRTL] Fix Grand Central and Layers Directories

Fix a bug involving interactions of Grand Central and layers output
directories.  When compiling a design that has a DUT and a testbench
directory, treat modules that are instantiated by Grand Central and under
a bound module (which is lowered from a layer) as belonging to Grand
Central and should be placed in its output directory.

This will be revisited in the future as the FIRRTL ABI moves more towards
filelists or manifests.

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

lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp

@@ -1711,27 +1711,42 @@ void GrandCentralPass::runOnOperation() {
   instancePaths = &instancePathCache;
 
   /// Contains the set of modules which are instantiated by the DUT, but not a
-  /// companion or instantiated by a companion.  If no DUT exists, treat the top
+  /// companion, instantiated by a companion, or instantiated under a bind.  If
-  /// module as if it were the DUT.  This works by doing a depth-first walk of
+  /// no DUT exists, treat the top module as if it were the DUT.  This works by
-  /// the instance graph, starting from the "effective" DUT and stopping the
+  /// doing a depth-first walk of the instance graph, starting from the
-  /// search at any modules which are known companions.
+  /// "effective" DUT and stopping the search at any modules which are known
-  DenseSet<igraph::ModuleOpInterface> dutModules;
+  /// companions or any instances which are marked "lowerToBind".
-  FModuleOp effectiveDUT = dut;
+  DenseSet<InstanceGraphNode *> dutModules;
-  if (!effectiveDUT)
+  InstanceGraphNode *effectiveDUT;
-    effectiveDUT = cast<FModuleOp>(
+  if (dut)
-        *instancePaths->instanceGraph.getTopLevelNode()->getModule());
+    effectiveDUT = instancePaths->instanceGraph.lookup(dut);
-  auto dfRange =
+  else
-      llvm::depth_first(instancePaths->instanceGraph.lookup(effectiveDUT));
+    effectiveDUT = instancePaths->instanceGraph.getTopLevelNode();
-  for (auto i = dfRange.begin(), e = dfRange.end(); i != e;) {
+  {
-    auto module = cast<FModuleLike>(*i->getModule());
+    SmallVector<InstanceGraphNode *> modules({effectiveDUT});
-    if (AnnotationSet(module).hasAnnotation(companionAnnoClass)) {
+    while (!modules.empty()) {
-      i.skipChildren();
+      auto *m = modules.pop_back_val();
-      continue;
+      for (InstanceRecord *a : *m) {
+        auto *mod = a->getTarget();
+        // Skip modules that we've visited, that are are under the companion
+        // module, or are bound/under a layer block.
+        if (auto block = a->getInstance()->getParentOfType<LayerBlockOp>()) {
+          auto diag = a->getInstance().emitOpError()
+                      << "is instantiated under a '" << block.getOperationName()
+                      << "' op which is unexpected by GrandCentral (did you "
+                         "forget to run the LowerLayers pass?)";
+          diag.attachNote(block.getLoc())
+              << "the '" << block.getOperationName() << "' op is here";
+          removalError = true;
+        }
+        if (dutModules.contains(mod) ||
+            AnnotationSet(mod->getModule()).hasAnnotation(companionAnnoClass) ||
+            cast<InstanceOp>(*a->getInstance()).getLowerToBind())
+          continue;
+        modules.push_back(mod);
+        dutModules.insert(mod);
+      }
     }
-    dutModules.insert(i->getModule<igraph::ModuleOpInterface>());
-    // Manually increment the iterator to avoid walking off the end from
-    // skipChildren.
-    ++i;
   }
 
   // Maybe return the lone instance of a module.  Generate errors on the op if
@@ -1765,7 +1780,6 @@ void GrandCentralPass::runOnOperation() {
   /// (2) the leafMap.  Annotations are removed as they are discovered and if
   /// they are not malformed.
   DenseSet<Operation *> modulesToDelete;
-  removalError = false;
   circuitOp.walk([&](Operation *op) {
     TypeSwitch<Operation *>(op)
         .Case<RegOp, RegResetOp, WireOp, NodeOp>([&](auto op) {
@@ -1950,12 +1964,11 @@ void GrandCentralPass::runOnOperation() {
                 // Check to see if we should change the output directory of a
                 // module.  Only update in the following conditions:
                 //   1) The module is the companion.
-                //   2) The module is NOT instantiated by the effective DUT.
+                //   2) The module is NOT instantiated by the effective DUT or
+                //      is under a bind.
                 auto *modNode = instancePaths->instanceGraph.lookup(mod);
                 SmallVector<InstanceRecord *> instances(modNode->uses());
-                if (modNode != companionNode &&
+                if (modNode != companionNode && dutModules.count(modNode))
-                    dutModules.count(
-                        modNode->getModule<igraph::ModuleOpInterface>()))
                   continue;
 
                 LLVM_DEBUG({

test/Dialect/FIRRTL/grand-central-errors.mlir

@@ -375,3 +375,36 @@ firrtl.circuit "CompanionWithOutputs"
     firrtl.instance dut @DUT()
   }
 }
+
+// -----
+
+firrtl.circuit "UnexpectedLayer"
+  attributes {annotations = [
+    {class = "sifive.enterprise.grandcentral.AugmentedBundleType",
+     defName = "Foo",
+     elements = [
+       {class = "sifive.enterprise.grandcentral.AugmentedBundleType",
+        defName = "Bar",
+        elements = [],
+        name = "bar"}],
+     id = 0 : i64,
+     name = "MyView"}]}  {
+  firrtl.layer @A bind {}
+  firrtl.module private @MyView_companion()
+    attributes {annotations = [{
+      class = "sifive.enterprise.grandcentral.ViewAnnotation.companion",
+      id = 0 : i64,
+      name = "MyView"}]} {}
+  firrtl.module private @Foo() {}
+  firrtl.module private @DUT() {
+    firrtl.instance MyView_companion  @MyView_companion()
+    // expected-note @below {{the 'firrtl.layerblock' op is here}}
+    firrtl.layerblock @A {
+      // expected-error @below {{'firrtl.instance' op is instantiated under a 'firrtl.layerblock' op}}
+      firrtl.instance foo @Foo()
+    }
+  }
+  firrtl.module @UnexpectedLayer() {
+    firrtl.instance dut @DUT()
+  }
+}

test/firtool/lower-layers-directories.fir

@@ -0,0 +1,132 @@
+; RUN: firtool %s | FileCheck %s --implicit-check-not FILE
+
+; This test checks output directory behavior of circuits with layers with other
+; features.  Generally, layer code is treated as "verification" code and should
+; be kept out of the design area when `firtool` is run with options where the
+; design area is explicitly specified.
+;
+; This test is expected to eventually be removed once the FIRRTL ABI switches to
+; using filelists or manifests and a flat output directory structure.
+
+FIRRTL version 4.0.0
+circuit Testbench: %[[
+  {
+    "class": "sifive.enterprise.grandcentral.ViewAnnotation",
+    "name": "MyView",
+    "companion": "~Testbench|GrandCentral",
+    "parent": "~Testbench|Testbench",
+    "view": {
+      "class": "sifive.enterprise.grandcentral.AugmentedBundleType",
+      "defName": "Interface",
+      "elements": [
+        {
+          "name": "uint",
+          "description": "a wire called 'uint'",
+          "tpe": {
+            "class": "sifive.enterprise.grandcentral.AugmentedGroundType",
+            "ref": {
+              "circuit": "Testbench",
+              "module": "DUT",
+              "path": [],
+              "ref": "a",
+              "component": []
+            },
+            "tpe": {
+              "class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
+            }
+          }
+        }
+      ]
+    }
+  },
+  {
+    "class": "sifive.enterprise.grandcentral.ExtractGrandCentralAnnotation",
+    "directory": "gct",
+    "filename": "gct/bindings.sv"
+  },
+  {
+    "class":"firrtl.transforms.DontTouchAnnotation",
+    "target":"~Testbench|DUT>a"
+  },
+  {
+    "class":"firrtl.transforms.DontTouchAnnotation",
+    "target":"~Testbench|Foo>a"
+  },
+  {
+    "class":"firrtl.transforms.DontTouchAnnotation",
+    "target":"~Testbench|Bar>a"
+  },
+  {
+    "class":"firrtl.transforms.DontTouchAnnotation",
+    "target":"~Testbench|Baz>a"
+  },
+  {
+    "class":"firrtl.transforms.DontTouchAnnotation",
+    "target":"~Testbench|Qux>a"
+  },
+  {
+    "class":"firrtl.transforms.DontTouchAnnotation",
+    "target":"~Testbench|Quz>a"
+  },
+  {
+    "class": "sifive.enterprise.firrtl.MarkDUTAnnotation",
+    "target": "~Testbench|DUT"
+  },
+  {
+    "class": "sifive.enterprise.firrtl.TestBenchDirAnnotation",
+    "dirname": "testbench"
+  }
+]]
+  layer A, bind:
+
+  module Foo:
+    node a = UInt<1>(0)
+  module Bar:
+    node a = UInt<1>(1)
+  module Baz:
+    node a = UInt<2>(2)
+
+  module Qux:
+    node a = UInt<2>(3)
+  module Quz:
+    node a = UInt<3>(4)
+
+  module GrandCentral:
+    inst qux of Qux
+    inst quz of Quz
+
+  module DUT:
+    inst foo of Foo
+    inst baz of Baz
+    inst grandCentral of GrandCentral
+
+    node a = UInt<3>(4)
+
+    layerblock A:
+      inst bar of Bar
+      inst bazz of Baz
+      inst quz of Quz
+
+  public module Testbench:
+    inst dut of DUT
+
+; The only modules NOT in a file are Foo, Baz, and DUT.
+;
+; CHECK: module Foo();
+; CHECK: module Baz();
+; CHECK: module DUT();
+
+; The following modules are all in the Testbench directory.
+;
+; CHECK-DAG: FILE "testbench{{[/\]}}Bar.sv"
+; CHECK-DAG: FILE "testbench{{[/\]}}DUT_A.sv"
+; CHECK-DAG: FILE "testbench{{[/\]}}Testbench.sv"
+; CHECK-DAG: FILE "testbench{{[/\]}}layers_Testbench_A.sv"
+
+; The following modules are all in the Grand Central directory.
+;
+; CHECK-DAG: FILE "gct{{[/\]}}GrandCentral.sv"
+; CHECK-DAG: FILE "gct{{[/\]}}Interface.sv"
+; CHECK-DAG: FILE "gct{{[/\]}}Qux.sv"
+; CHECK-DAG: FILE "gct{{[/\]}}Quz.sv"
+; CHECK-DAG: FILE "gct{{[/\]}}bindings.sv"