[FIRRTL] Add linting of XMRs in the design

Extend the FIRRTL linter to check that there are no XMRs that will be
located in the "design".  This intentionally only checks for problems in
the "design" and NOT the "effective design".  This errs on the side of
having more false negatives without introducing the possibility of false
positives for separate compilation flows.

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

lib/Analysis/FIRRTLInstanceInfo.cpp

@@ -16,6 +16,7 @@
 #include "circt/Analysis/FIRRTLInstanceInfo.h"
 #include "circt/Dialect/FIRRTL/AnnotationDetails.h"
 #include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h"
+#include "circt/Dialect/SV/SVOps.h"
 #include "circt/Support/Debug.h"
 #include "circt/Support/InstanceGraph.h"
 #include "llvm/ADT/PostOrderIterator.h"
@@ -143,7 +144,8 @@ InstanceInfo::InstanceInfo(Operation *op, mlir::AnalysisManager &am) {
         bool underLayer = false;
         if (auto instanceOp = useIt->getInstance<InstanceOp>()) {
           if (instanceOp.getLowerToBind() || instanceOp.getDoNotPrint() ||
-              instanceOp->getParentOfType<LayerBlockOp>())
+              instanceOp->getParentOfType<LayerBlockOp>() ||
+              instanceOp->getParentOfType<sv::IfDefOp>())
             underLayer = true;
         }
 

lib/Dialect/FIRRTL/Transforms/Lint.cpp

@@ -6,9 +6,11 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "circt/Analysis/FIRRTLInstanceInfo.h"
 #include "circt/Dialect/FIRRTL/FIRRTLOps.h"
 #include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
 #include "circt/Dialect/FIRRTL/Passes.h"
+#include "circt/Dialect/SV/SVOps.h"
 #include "mlir/Pass/Pass.h"
 #include "llvm/ADT/APSInt.h"
 
@@ -37,8 +39,8 @@ struct Config {
 class Linter {
 
 public:
-  Linter(FModuleOp fModule, const Config &config)
-      : fModule(fModule), config(config){};
+  Linter(FModuleOp fModule, InstanceInfo &instanceInfo, const Config &config)
+      : fModule(fModule), instanceInfo(instanceInfo), config(config){};
 
   /// Lint the specified module.
   LogicalResult lint() {
@@ -50,6 +52,10 @@ public:
         if (config.lintStaticAsserts && checkAssert(op).failed())
           failed = true;
 
+      if (auto xmrDerefOp = dyn_cast<XMRDerefOp>(op))
+        if (checkXmr(xmrDerefOp).failed())
+          failed = true;
+
       return WalkResult::advance();
     });
 
@@ -61,6 +67,7 @@ public:
 
 private:
   FModuleOp fModule;
+  InstanceInfo &instanceInfo;
   const Config &config;
 
   LogicalResult checkAssert(Operation *op) {
@@ -93,6 +100,45 @@ private:
 
     return success();
   }
+
+  LogicalResult checkXmr(XMRDerefOp op) {
+    // XMRs under layers are okay.
+    if (op->getParentOfType<LayerBlockOp>() ||
+        op->getParentOfType<sv::IfDefOp>())
+      return success();
+
+    // The XMR is not under a layer.  This module must never be instantiated in
+    // the design.  Intentionally do NOT use "effective" design as this could
+    // lead to false positives.
+    if (!instanceInfo.anyInstanceInDesign(fModule))
+      return success();
+
+    // If all users are connect sources, and each connect destinations is to an
+    // instance which is marked `lowerToBind`, then this is a pattern for
+    // inlining the XMR into the bound instance site.  This pattern is used by
+    // Grand Central, but not elsewhere.
+    //
+    // If there are _no_ users, this is also okay as this expression will not be
+    // emitted.
+    auto boundInstancePortUser = [&](auto user) {
+      auto connect = dyn_cast<MatchingConnectOp>(user);
+      if (connect && connect.getSrc() == op.getResult())
+        if (auto *definingOp = connect.getDest().getDefiningOp())
+          if (auto instanceOp = dyn_cast<InstanceOp>(definingOp))
+            if (instanceOp->hasAttr("lowerToBind"))
+              return true;
+      return false;
+    };
+    if (llvm::all_of(op.getResult().getUsers(), boundInstancePortUser))
+      return success();
+
+    auto diag =
+        op.emitOpError()
+        << "is in the design. (Did you forget to put it under a layer?)";
+    diag.attachNote(fModule.getLoc()) << "op is instantiated in this module";
+
+    return failure();
+  }
 };
 
 struct LintPass : public circt::firrtl::impl::LintBase<LintPass> {
@@ -101,6 +147,7 @@ struct LintPass : public circt::firrtl::impl::LintBase<LintPass> {
   void runOnOperation() override {
 
     CircuitOp circuitOp = getOperation();
+    auto instanceInfo = getAnalysis<InstanceInfo>();
 
     auto reduce = [](LogicalResult a, LogicalResult b) -> LogicalResult {
       if (succeeded(a) && succeeded(b))
@@ -108,7 +155,7 @@ struct LintPass : public circt::firrtl::impl::LintBase<LintPass> {
       return failure();
     };
     auto transform = [&](FModuleOp moduleOp) -> LogicalResult {
-      return Linter(moduleOp, {lintStaticAsserts}).lint();
+      return Linter(moduleOp, instanceInfo, {lintStaticAsserts}).lint();
     };
 
     SmallVector<FModuleOp> modules(circuitOp.getOps<FModuleOp>());

test/Dialect/FIRRTL/lint-noerror.mlir

@@ -0,0 +1,100 @@
+// RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(firrtl-lint))' %s
+
+// This test checks that the linter does _not_ error for certain patterns.
+
+firrtl.circuit "XmrNoDut" {
+  hw.hierpath private @xmrPath [@XmrNoDut::@sym]
+  firrtl.module @XmrNoDut() {
+    %a = firrtl.wire sym @sym : !firrtl.uint<1>
+    %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+    %b = firrtl.node %0 : !firrtl.uint<1>
+  }
+}
+
+firrtl.circuit "XmrInTestHarness" {
+  hw.hierpath private @xmrPath [@XmrInTestHarness::@sym]
+  firrtl.module @Dut() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.firrtl.MarkDUTAnnotation"
+      }
+    ]
+  } {
+  }
+  firrtl.module @XmrInTestHarness() {
+    firrtl.instance dut @Dut()
+    %a = firrtl.wire sym @sym : !firrtl.uint<1>
+    %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+    %b = firrtl.node %0 : !firrtl.uint<1>
+  }
+}
+
+firrtl.circuit "XmrInLayer" {
+  hw.hierpath private @xmrPath [@XmrInLayer::@sym]
+  firrtl.layer @A bind {}
+  firrtl.module @XmrInLayer() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.firrtl.MarkDUTAnnotation"
+      }
+    ]
+  } {
+    firrtl.layerblock @A {
+      %a = firrtl.wire sym @sym : !firrtl.uint<1>
+      %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+      %b = firrtl.node %0 : !firrtl.uint<1>
+    }
+  }
+}
+
+firrtl.circuit "XmrInLayer_lowered_Bind" {
+  hw.hierpath private @xmrPath [@Foo::@sym]
+  firrtl.module @Foo() {
+    %a = firrtl.wire sym @sym : !firrtl.uint<1>
+    %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+    %b = firrtl.node %0 : !firrtl.uint<1>
+  }
+  firrtl.module @XmrInLayer_lowered_Bind() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.firrtl.MarkDUTAnnotation"
+      }
+    ]
+  } {
+    firrtl.instance a sym @a {
+      doNotPrint
+    } @Foo()
+  }
+}
+
+firrtl.circuit "XmrInLayer_lowered_IfDef" {
+  hw.hierpath private @xmrPath [@XmrInLayer_lowered_IfDef::@sym]
+  sv.macro.decl @layer
+  firrtl.module @XmrInLayer_lowered_IfDef() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.firrtl.MarkDUTAnnotation"
+      }
+    ]
+  } {
+    sv.ifdef @layer {
+      %a = firrtl.wire sym @sym : !firrtl.uint<1>
+      %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+      %b = firrtl.node %0 : !firrtl.uint<1>
+    }
+  }
+}
+
+firrtl.circuit "XMRInDesignNoUsers" {
+  hw.hierpath private @xmrPath [@XMRInDesignNoUsers::@sym]
+  firrtl.module @XMRInDesignNoUsers() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.firrtl.MarkDUTAnnotation"
+      }
+    ]
+  } {
+    %a = firrtl.wire sym @sym : !firrtl.uint<1>
+    %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+  }
+}

test/Dialect/FIRRTL/lint.mlir

@@ -82,3 +82,48 @@ firrtl.layer @GroupFoo bind {}
     }
   }
 }
+
+// -----
+
+firrtl.circuit "XMRInDesign" {
+  hw.hierpath private @xmrPath [@XMRInDesign::@sym]
+  // expected-note @below {{op is instantiated in this module}}
+  firrtl.module @XMRInDesign() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.firrtl.MarkDUTAnnotation"
+      }
+    ]
+  } {
+    %a = firrtl.wire sym @sym : !firrtl.uint<1>
+    // expected-error @below {{is in the design. (Did you forget to put it under a layer?)}}
+    %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+    %b = firrtl.node %0 : !firrtl.uint<1>
+  }
+}
+
+// -----
+
+firrtl.circuit "XMRInDesignAndTestHarness" {
+  hw.hierpath private @xmrPath [@Foo::@sym]
+  // expected-note @below {{op is instantiated in this module}}
+  firrtl.module @Foo() {
+    %a = firrtl.wire sym @sym : !firrtl.uint<1>
+    // expected-error @below {{is in the design. (Did you forget to put it under a layer?)}}
+    %0 = firrtl.xmr.deref @xmrPath : !firrtl.uint<1>
+    %b = firrtl.node %0 : !firrtl.uint<1>
+  }
+  firrtl.module @DUT() attributes {
+    annotations = [
+      {
+        class = "sifive.enterprise.firrtl.MarkDUTAnnotation"
+      }
+    ]
+  } {
+    firrtl.instance foo @Foo()
+  }
+  firrtl.module @XMRInDesignAndTestHarness()  {
+    firrtl.instance dut @DUT()
+    firrtl.instance foo @Foo()
+  }
+}