[FIRRTL] Rewrite, extend InstanceInfo for GC Views

Rewrite the `CheckLayers` pass to more directly rely on the `InstanceInfo`
analysis.  Instead of the top-down walk (which was there from the original
architecture which did not have access to `InstanceInfo` and was
originally pushing layer information down), use only a bottom-up walk and
rely on the `InstanceInfo` analysis to determine when modules could be
problematic.  This preserves a similar style of the original verbose
errors/notes which reports transitive errors (modules which do not contain
layers, but instantiate modules that do), but switches to something that
is easier to work with.

Extend the `CheckLayers` pass to now properly handle Grand Central
companion modules.  These are conceptually the same as layers and are
now lumped under the "under layer" bucket by `InstanceInfo`.

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

Author: seldridge

lib/Dialect/FIRRTL/Transforms/CheckLayers.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "circt/Analysis/FIRRTLInstanceInfo.h"
+#include "circt/Dialect/FIRRTL/AnnotationDetails.h"
 #include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h"
 #include "circt/Dialect/FIRRTL/FIRRTLOps.h"
 #include "circt/Dialect/FIRRTL/Passes.h"
@@ -31,44 +32,79 @@ class CheckLayers {
   CheckLayers(InstanceGraph &instanceGraph, InstanceInfo &instanceInfo)
       : iGraph(instanceGraph), iInfo(instanceInfo) {}
 
-  /// Walk a module, reporting any illegal instantation of layers under layers,
-  /// and record if this module contains any layerblocks.
+  /// Walk a module and record any illegal layerblocks/Grand Central companions
+  /// under layerblocks/Grand Central companions.  This function should be run
+  /// on children before parents for accurate reporting.
   void run(FModuleOp moduleOp) {
-    // No instance is under a layer block.  No further examination is necessary.
+
+    // The module is _never_ instantiated under a layer.  There is nothing to do
+    // because erroneous instantiations are reported when examining the module.
+    // Note: Grand Central companions are under a layer (because InstanceInfo
+    // uses the inclusive definition of "under" to be consistent with how the
+    // design-under-test module is "under" the design).
     if (!iInfo.anyInstanceUnderLayer(moduleOp))
       return;
 
-    // The module is under a layer block.  Verify that it has no layer blocks.
-    LayerBlockOp layerBlockOp;
-    moduleOp.getBodyBlock()->walk([&](LayerBlockOp op) {
-      layerBlockOp = op;
-      return WalkResult::interrupt();
+    // Check if this module has any layerblock ops.  If these exist, then these
+    // may be errors.
+    SmallVector<Operation *> layerBlockOps;
+    moduleOp->walk([&](LayerBlockOp layerBlockOp) {
+      layerBlockOps.push_back(layerBlockOp);
     });
-    if (!layerBlockOp)
+
+    bool isGCCompanion =
+        AnnotationSet::hasAnnotation(moduleOp, companionAnnoClass);
+
+    // Both Grand Central copmanions and modules that transitively instantiate
+    // layerblocks/Grand Central companions require analysis of their
+    // instantiation sites.  However, if this is a normal module instantiated
+    // under a layer and it contains no layerblocks, then early exit to avoid
+    // unnecessarily examining instantiation sites.
+    if (!isGCCompanion && !transitiveModules.contains(moduleOp) &&
+        layerBlockOps.empty())
       return;
 
-    // The module contains layer blocks and is instantiated under a layer block.
-    // Walk up the instance hierarchy to find the first instance which is
-    // directly under a layer block.
-    error = true;
-    for (auto *node : llvm::inverse_depth_first(iGraph.lookup(moduleOp))) {
-      auto modOp = node->getModule();
-      if (previousErrors.contains(node) || !iInfo.anyInstanceUnderLayer(modOp))
-        continue;
-      for (auto *instNode : node->uses()) {
-        auto instanceOp = instNode->getInstance();
-        if (instanceOp->getParentOfType<LayerBlockOp>()) {
-          auto moduleName = modOp.getModuleNameAttr();
-          auto diag = emitError(instanceOp.getLoc())
-                      << "cannot instantiate " << moduleName
-                      << " under a layerblock, because " << moduleName
-                      << " contains a layerblock";
-          diag.attachNote(layerBlockOp->getLoc()) << "layerblock here";
-          previousErrors.insert(node);
-          continue;
-        }
+    // Record instantiations of this module under layerblocks or modules that
+    // are under layer blocks.  Update transitive modules.
+    SmallVector<Operation *> instUnderLayerBlock, instUnderLayerModule;
+    for (auto *instNode : iGraph.lookup(moduleOp)->uses()) {
+      auto *instOp = instNode->getInstance().getOperation();
+      if (instOp->getParentOfType<LayerBlockOp>())
+        instUnderLayerBlock.push_back(instOp);
+      else if (auto parent = instOp->getParentOfType<FModuleOp>();
+               iInfo.anyInstanceUnderLayer(parent)) {
+        transitiveModules.insert(parent);
+        instUnderLayerModule.push_back(instOp);
       }
     }
+
+    // The module _may_ contain no errors if it is a Grand Central companion or
+    // a transitive module.  Do a final check to ensure that an error exists.
+    if (layerBlockOps.empty() && instUnderLayerBlock.empty() &&
+        instUnderLayerModule.empty())
+      return;
+
+    // Record that an error occurred and print out an error message on the
+    // module with notes for more information.
+    error = true;
+    auto diag = moduleOp->emitOpError();
+    if (isGCCompanion)
+      diag
+          << "is a Grand Central companion that either contains layerblocks or";
+
+    else
+      diag << "either contains layerblocks or";
+    diag << " has at least one instance that is or contains a Grand Central "
+            "companion or layerblocks";
+
+    for (auto *layerBlockOp : layerBlockOps)
+      diag.attachNote(layerBlockOp->getLoc()) << "illegal layerblock here";
+    for (auto *instUnderLayerBlock : instUnderLayerBlock)
+      diag.attachNote(instUnderLayerBlock->getLoc())
+          << "illegal instantiation under a layerblock here";
+    for (auto *instUnderLayerModule : instUnderLayerModule)
+      diag.attachNote(instUnderLayerModule->getLoc())
+          << "illegal instantiation in a module under a layer here";
   }
 
 public:
@@ -77,7 +113,7 @@ class CheckLayers {
     CheckLayers checkLayers(instanceGraph, instanceInfo);
     DenseSet<InstanceGraphNode *> visited;
     for (auto *root : instanceGraph) {
-      for (auto *node : llvm::inverse_post_order_ext(root, visited)) {
+      for (auto *node : llvm::post_order_ext(root, visited)) {
         if (auto moduleOp = dyn_cast<FModuleOp>(node->getModule<Operation *>()))
           checkLayers.run(moduleOp);
       }
@@ -90,9 +126,11 @@ class CheckLayers {
   InstanceGraph &iGraph;
   InstanceInfo &iInfo;
 
-  /// This records modules for which we have already generated errors when doing
-  /// a top-down walk.
-  DenseSet<const InstanceGraphNode *> previousErrors;
+  /// A module whose instances (transitively) contain layerblocks or Grand
+  /// Central companions.  This is used so that every illegal instantiation can
+  /// be reported.  This is populated by `run` and requires child modules to be
+  /// visited before parents.
+  DenseSet<Operation *> transitiveModules;
 
   /// Indicates if this checker found an error.
   bool error = false;

test/Dialect/FIRRTL/check-layers-errors.mlir

@@ -4,12 +4,13 @@ firrtl.circuit "Simple" {
   firrtl.layer @A bind {}
   firrtl.module @Simple() {
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "Layers" under a layerblock, because "Layers" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layers @Layers()
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Layers() {
-    // expected-note @below {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
   }
 }
@@ -20,15 +21,18 @@ firrtl.circuit "Transitive" {
   firrtl.layer @A bind {}
   firrtl.module @Transitive() {
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "Middle" under a layerblock, because "Middle" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance middle @Middle()
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Middle() {
+    // expected-note @below {{illegal instantiation in a module under a layer here}}
     firrtl.instance layers @Layers()
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Layers() {
-    // expected-note @below {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
   }
 }
@@ -39,13 +43,15 @@ firrtl.circuit "FirstLayerBLockFound" {
   firrtl.layer @A bind {}
   firrtl.module @FirstLayerBLockFound() {
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "Layers" under a layerblock, because "Layers" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layers @Layers()
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Layers() {
-    // expected-note @below {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
   }
 }
@@ -56,40 +62,44 @@ firrtl.circuit "MultipleErrors" {
   firrtl.layer @A bind {}
   firrtl.module @MultipleErrors() {
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "Layers1" under a layerblock, because "Layers1" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layers1 @Layers1()
-      // expected-error @below {{cannot instantiate "Layers2" under a layerblock, because "Layers2" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layers2 @Layers2()
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Layers1() {
-    // expected-note @below {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Layers2() {
-    // expected-note @below {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
   }
 }
 
+
 // -----
 
 firrtl.circuit "MultipleErrors" {
   firrtl.layer @A bind {}
   firrtl.module @MultipleErrors() {
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "Layers" under a layerblock, because "Layers" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layers1 @Layers()
     }
   }
   firrtl.module @OtherTop() {
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "Layers" under a layerblock, because "Layers" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layers1 @Layers()
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Layers() {
-    // expected-note @+1 {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
   }
 }
@@ -100,19 +110,21 @@ firrtl.circuit "NestedLayers" {
   firrtl.layer @A bind {}
   firrtl.module @NestedLayers() {
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "LayerA" under a layerblock, because "LayerA" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layera @LayerA()
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @LayerA() {
-    // expected-note @below {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {
-      // expected-error @below {{cannot instantiate "LayerB" under a layerblock, because "LayerB" contains a layerblock}}
+      // expected-note @below {{illegal instantiation under a layerblock here}}
       firrtl.instance layerb @LayerB()
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @LayerB() {
-    // expected-note @below {{layerblock here}}
+    // expected-note @below {{illegal layerblock here}}
     firrtl.layerblock @A {}
   }
 }
@@ -124,17 +136,67 @@ firrtl.circuit "RegionOps" {
   firrtl.module @RegionOps(in %in : !firrtl.uint<1>) {
     firrtl.when %in : !firrtl.uint<1> {
       firrtl.layerblock @A {
-        // expected-error @below {{cannot instantiate "Layers" under a layerblock, because "Layers" contains a layerblock}}
+        // expected-note @below {{illegal instantiation under a layerblock here}}
         %layers_in = firrtl.instance layers @Layers(in in : !firrtl.enum<a: uint<1>>)
       }
     }
   }
+  // expected-error @below {{either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
   firrtl.module @Layers(in %in : !firrtl.enum<a: uint<1>>) {
     firrtl.match %in : !firrtl.enum<a: uint<1>> {
       case a(%arg0) {
-        // expected-note @below {{layerblock here}}
+        // expected-note @below {{illegal layerblock here}}
         firrtl.layerblock @A {}
       }
     }
   }
 }
+
+// -----
+
+// A Grand Central companion cannot contain layerblocks.
+firrtl.circuit "Foo" {
+  firrtl.layer @A bind {}
+  // expected-error @below {{is a Grand Central companion that either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
+  firrtl.module @Bar() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.grandcentral.ViewAnnotation.companion",
+        defName = "GroundView",
+        id = 0 : i64,
+        name = "GroundView"
+      }
+    ]
+  } {
+    // expected-note @below {{illegal layerblock here}}
+    firrtl.layerblock @A {}
+  }
+  firrtl.module @Foo() {
+    firrtl.instance bar @Bar()
+  }
+}
+
+// -----
+
+// A Grand Central companion cannot contain layerblocks.
+firrtl.circuit "Foo" {
+  firrtl.layer @A bind {}
+  // expected-error @below {{is a Grand Central companion that either contains layerblocks or has at least one instance that is or contains a Grand Central companion or layerblocks}}
+  firrtl.module @Bar() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.grandcentral.ViewAnnotation.companion",
+        defName = "GroundView",
+        id = 0 : i64,
+        name = "GroundView"
+      }
+    ]
+  } {
+  }
+  firrtl.module @Foo() {
+    firrtl.layerblock @A {
+      // expected-note @below {{illegal instantiation under a layerblock here}}
+      firrtl.instance bar @Bar()
+    }
+  }
+}