[FIRRTL] Avoid InstanceGraphNode* in InstanceInfo (#7643)

Fix a problem with the InstanceInfo analysis stemming from the use of
`InstanceGraphNode *` to track the design-under-test (DUT) and "effective"
DUT.  If the instance graph changed, then these would be invalidated.
Instead use a `igraph::ModuleOpInterface` which is going to be stable.

Signed-off-by: Schuyler Eldridge <[email protected]>

include/circt/Analysis/FIRRTLInstanceInfo.h

@@ -77,10 +77,10 @@ class InstanceInfo {
   /// Information about a circuit
   struct CircuitAttributes {
     /// The design-under-test if one is defined.
-    igraph::InstanceGraphNode *dutNode;
+    igraph::ModuleOpInterface dut;
 
     /// The design-under-test if one is defined or the top module.
-    igraph::InstanceGraphNode *effectiveDutNode;
+    igraph::ModuleOpInterface effectiveDut;
   };
 
   /// Information about a module
@@ -101,12 +101,12 @@ class InstanceInfo {
 
   /// Return the design-under-test if one is defined for the circuit, otherwise
   /// return null.
-  igraph::InstanceGraphNode *getDut();
+  igraph::ModuleOpInterface getDut();
 
   /// Return the "effective" design-under-test.  This will be the
   /// design-under-test if one is defined.  Otherwise, this will be the root
   /// node of the instance graph.
-  igraph::InstanceGraphNode *getEffectiveDut();
+  igraph::ModuleOpInterface getEffectiveDut();
 
   //===--------------------------------------------------------------------===//
   // Module Attribute Queries
@@ -150,8 +150,8 @@ class InstanceInfo {
 
 private:
   /// Stores circuit-level attributes.
-  CircuitAttributes circuitAttributes = {/*dutNode=*/nullptr,
-                                         /*effectiveDutNode=*/nullptr};
+  CircuitAttributes circuitAttributes = {/*dut=*/nullptr,
+                                         /*effectiveDut=*/nullptr};
 
   /// Internal mapping of operations to module attributes.
   DenseMap<Operation *, ModuleAttributes> moduleAttributes;

lib/Analysis/FIRRTLInstanceInfo.cpp

@@ -69,7 +69,7 @@ void InstanceInfo::LatticeValue::mergeIn(bool value) {
 InstanceInfo::InstanceInfo(Operation *op, mlir::AnalysisManager &am) {
   auto &iGraph = am.getAnalysis<InstanceGraph>();
 
-  circuitAttributes.effectiveDutNode = iGraph.getTopLevelNode();
+  circuitAttributes.effectiveDut = iGraph.getTopLevelNode()->getModule();
 
   // Visit modules in reverse post-order (visit parents before children) because
   // information flows in this direction---the attributes of modules are
@@ -83,8 +83,8 @@ InstanceInfo::InstanceInfo(Operation *op, mlir::AnalysisManager &am) {
       // Set DUT-related attributes.
       auto isDut = AnnotationSet(moduleOp).hasAnnotation(dutAnnoClass);
       if (isDut) {
-        circuitAttributes.dutNode = modIt;
-        circuitAttributes.effectiveDutNode = modIt;
+        circuitAttributes.dut = modIt->getModule();
+        circuitAttributes.effectiveDut = modIt->getModule();
       }
 
       // If the module is not instantiated, then set attributes and early exit.
@@ -120,13 +120,13 @@ InstanceInfo::InstanceInfo(Operation *op, mlir::AnalysisManager &am) {
         << llvm::indent(2) << "circuit attributes:\n"
         << llvm::indent(4) << "hasDut: " << (hasDut() ? "true" : "false")
         << "\n"
-        << llvm::indent(4) << "dutNode: ";
-    if (auto dutNode = circuitAttributes.dutNode)
-      dutNode->getModule()->print(llvm::dbgs(), flags);
+        << llvm::indent(4) << "dut: ";
+    if (auto dut = circuitAttributes.dut)
+      dut->print(llvm::dbgs(), flags);
     else
       llvm::dbgs() << "null";
-    llvm::dbgs() << "\n" << llvm::indent(4) << "effectiveDutNode: ";
-    circuitAttributes.effectiveDutNode->getModule()->print(llvm::dbgs(), flags);
+    llvm::dbgs() << "\n" << llvm::indent(4) << "effectiveDut: ";
+    circuitAttributes.effectiveDut->print(llvm::dbgs(), flags);
     llvm::dbgs() << "\n" << llvm::indent(2) << "module attributes:\n";
     for (auto *node : llvm::depth_first(iGraph.getTopLevelNode())) {
       auto moduleOp = node->getModule();
@@ -149,26 +149,26 @@ InstanceInfo::getModuleAttributes(igraph::ModuleOpInterface op) {
   return moduleAttributes.find(op)->getSecond();
 }
 
-bool InstanceInfo::hasDut() { return circuitAttributes.dutNode; }
+bool InstanceInfo::hasDut() { return circuitAttributes.dut; }
 
 bool InstanceInfo::isDut(igraph::ModuleOpInterface op) {
   if (hasDut())
-    return op == circuitAttributes.dutNode->getModule();
+    return op == circuitAttributes.dut;
   return false;
 }
 
 bool InstanceInfo::isEffectiveDut(igraph::ModuleOpInterface op) {
   if (hasDut())
     return isDut(op);
-  return op == circuitAttributes.effectiveDutNode->getModule();
+  return op == circuitAttributes.effectiveDut;
 }
 
-igraph::InstanceGraphNode *InstanceInfo::getDut() {
-  return circuitAttributes.dutNode;
+igraph::ModuleOpInterface InstanceInfo::getDut() {
+  return circuitAttributes.dut;
 }
 
-igraph::InstanceGraphNode *InstanceInfo::getEffectiveDut() {
-  return circuitAttributes.effectiveDutNode;
+igraph::ModuleOpInterface InstanceInfo::getEffectiveDut() {
+  return circuitAttributes.effectiveDut;
 }
 
 bool InstanceInfo::anyInstanceUnderDut(igraph::ModuleOpInterface op) {

lib/Analysis/TestPasses.cpp

@@ -213,13 +213,13 @@ static void printCircuitInfo(firrtl::CircuitOp op,
   llvm::errs() << "\n"
                << "    hasDut: " << iInfo.hasDut() << "\n"
                << "    dut: ";
-  if (auto *dutNode = iInfo.getDut())
-    dutNode->getModule()->print(llvm::errs(), flags);
+  if (auto dutNode = iInfo.getDut())
+    dutNode->print(llvm::errs(), flags);
   else
     llvm::errs() << "null";
   llvm::errs() << "\n"
                << "    effectiveDut: ";
-  iInfo.getEffectiveDut()->getModule()->print(llvm::errs(), flags);
+  iInfo.getEffectiveDut()->print(llvm::errs(), flags);
   llvm::errs() << "\n";
 }
 

lib/Dialect/FIRRTL/Transforms/AddSeqMemPorts.cpp

@@ -460,8 +460,6 @@ void AddSeqMemPortsPass::runOnOperation() {
   }
   extraPortsAttr = ArrayAttr::get(context, extraPorts);
 
-  auto *effectiveDut = instanceInfo->getEffectiveDut();
-
   // If there are no user ports, don't do anything.
   if (!userPorts.empty()) {
     // Update ports statistic.
@@ -471,7 +469,8 @@ void AddSeqMemPortsPass::runOnOperation() {
     // design-under-test. Skip any modules that are wholly instantiated under
     // layers.  If any memories are partially instantiated under a layer then
     // error.
-    for (auto *node : llvm::post_order(instanceInfo->getEffectiveDut())) {
+    for (auto *node : llvm::post_order(
+             instanceGraph->lookup(instanceInfo->getEffectiveDut()))) {
       auto op = node->getModule();
 
       // Skip anything wholly under a layer.
@@ -489,12 +488,13 @@ void AddSeqMemPortsPass::runOnOperation() {
     }
 
     // We handle the DUT differently than the rest of the modules.
-    if (auto dut = dyn_cast<FModuleOp>(effectiveDut->getModule())) {
+    auto effectiveDut = instanceInfo->getEffectiveDut();
+    if (auto *dut = dyn_cast<FModuleOp>(&effectiveDut)) {
       // For each instance of the dut, add the instance ports, but tie the port
       // to 0 instead of wiring them to the parent.
-      for (auto *instRec : effectiveDut->uses()) {
+      for (auto *instRec : instanceGraph->lookup(effectiveDut)->uses()) {
         auto inst = cast<InstanceOp>(*instRec->getInstance());
-        auto &dutMemInfo = memInfoMap[dut];
+        auto &dutMemInfo = memInfoMap[*dut];
         // Find out how many memory ports we have to add.
         auto &subExtraPorts = dutMemInfo.extraPorts;
         // If there are no extra ports, we don't have to do anything.
@@ -528,7 +528,7 @@ void AddSeqMemPortsPass::runOnOperation() {
 
   // If there is an output file, create it.
   if (outputFile)
-    createOutputFile(effectiveDut->getModule<igraph::ModuleOpInterface>());
+    createOutputFile(instanceInfo->getEffectiveDut());
 
   if (anythingChanged)
     markAnalysesPreserved<InstanceGraph>();