[FIRRTL] Add a new op interface for combinational loop detection. (#…

…7120)

Add a new CombDataflow op interface to FIRRTL.
This interface is used for specifying the combinational dataflow that exists in
 the results and operands of an operation. Any operation that doesn't implement
 this interface is assumed to have a combinational dependence from each operand
 to each result.
Currently only FIRRTL register and memory ops implement this interface, but it
 can be used in other dialects that intend to use the CheckCombCycles pass.

include/circt/Dialect/FIRRTL/FIRRTLDeclarations.td

@@ -235,7 +235,7 @@ def InstanceChoiceOp : HardwareDeclOp<"instance_choice", [
 }
 
 
-def MemOp : HardwareDeclOp<"mem"> {
+def MemOp : HardwareDeclOp<"mem", [CombDataflow]> {
   let summary = "Define a new mem";
   let arguments =
     (ins ConfinedAttr<I32Attr, [IntMinValue<0>]>:$readLatency,
@@ -329,6 +329,8 @@ def MemOp : HardwareDeclOp<"mem"> {
 
     // Extract the relevant attributes from the MemOp and return a FirMemory object.
     FirMemory getSummary();
+
+    SmallVector<std::pair<circt::FieldRef, circt::FieldRef>> computeDataFlow();
   }];
 }
 
@@ -388,7 +390,7 @@ def NodeOp : HardwareDeclOp<"node", [
   let hasFolder = 1;
 }
 
-def RegOp : HardwareDeclOp<"reg", [Forceable]> {
+def RegOp : HardwareDeclOp<"reg", [Forceable, CombDataflow]> {
   let summary = "Define a new register";
   let description = [{
     Declare a new register:
@@ -480,7 +482,7 @@ def RegOp : HardwareDeclOp<"reg", [Forceable]> {
   let hasCanonicalizeMethod = true;
 }
 
-def RegResetOp : HardwareDeclOp<"regreset", [Forceable]> {
+def RegResetOp : HardwareDeclOp<"regreset", [Forceable, CombDataflow]> {
   let summary = "Define a new register with a reset";
   let description = [{
     Declare a new register:

include/circt/Dialect/FIRRTL/FIRRTLOpInterfaces.h

@@ -19,6 +19,7 @@
 #include "circt/Dialect/HW/HWAttributes.h"
 #include "circt/Dialect/HW/HWOpInterfaces.h"
 #include "circt/Dialect/HW/InnerSymbolTable.h"
+#include "circt/Support/FieldRef.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/OpDefinition.h"

include/circt/Dialect/FIRRTL/FIRRTLOpInterfaces.td

@@ -523,4 +523,24 @@ def Forceable : OpInterface<"Forceable"> {
   }];
 }
 
+def CombDataflow : OpInterface<"CombDataFlow"> {
+  let cppNamespace = "circt::firrtl";
+  let description = [{
+    This interface is used for specifying the combinational dataflow that exists
+    in the results and operands of an operation.
+    Any operation that doesn't implement this interface is assumed to have a
+    combinational dependence from each operand to each result.
+  }];
+  let methods = [
+    InterfaceMethod<"Get the combinational dataflow relations between the"
+     "operands and the results. This returns a pair of fieldrefs. The first" 
+     "element is the destination and the second is the source of the dependence."
+     "The default implementation returns an empty list, which implies that the"
+     "operation is not combinational.",
+     "SmallVector<std::pair<circt::FieldRef, circt::FieldRef>>",
+     "computeDataFlow", (ins), [{}], /*defaultImplementation=*/[{
+      return {};
+    }]>,
+  ];
+}
 #endif // CIRCT_DIALECT_FIRRTL_FIRRTLOPINTERFACES_TD

lib/Dialect/FIRRTL/FIRRTLOps.cpp

@@ -113,6 +113,28 @@ bool firrtl::isDuplexValue(Value val) {
   return false;
 }
 
+SmallVector<std::pair<circt::FieldRef, circt::FieldRef>>
+MemOp::computeDataFlow() {
+  // If read result has non-zero latency, then no combinational dependency
+  // exists.
+  if (getReadLatency() > 0)
+    return {};
+
+  SmallVector<std::pair<circt::FieldRef, circt::FieldRef>> deps;
+  // Add a dependency from the enable and address fields to the data field.
+  for (auto memPort : getResults())
+    if (auto type = type_dyn_cast<BundleType>(memPort.getType())) {
+      auto enableFieldId = type.getFieldID((unsigned)ReadPortSubfield::en);
+      auto addressFieldId = type.getFieldID((unsigned)ReadPortSubfield::addr);
+      auto dataFieldId = type.getFieldID((unsigned)ReadPortSubfield::data);
+      deps.push_back({FieldRef(memPort, static_cast<unsigned>(dataFieldId)),
+                      FieldRef(memPort, static_cast<unsigned>(enableFieldId))});
+      deps.push_back({{memPort, static_cast<unsigned>(dataFieldId)},
+                      {memPort, static_cast<unsigned>(addressFieldId)}});
+    }
+  return deps;
+}
+
 /// Return the kind of port this is given the port type from a 'mem' decl.
 static MemOp::PortKind getMemPortKindFromType(FIRRTLType type) {
   constexpr unsigned int addr = 1 << 0;

lib/Dialect/FIRRTL/Transforms/CheckCombLoops.cpp

@@ -86,10 +86,14 @@ class DiscoverLoops {
                       });
     }
 
-    bool foreignOps = false;
     walk(module, [&](Operation *op) {
       llvm::TypeSwitch<Operation *>(op)
-          .Case<RegOp, RegResetOp>([&](auto) {})
+          .Case<CombDataFlow>([&](CombDataFlow df) {
+            // computeDataFlow returns a pair of FieldRefs, first element is the
+            // destination and the second is the source.
+            for (auto &dep : df.computeDataFlow())
+              addDrivenBy(dep.first, dep.second);
+          })
           .Case<Forceable>([&](Forceable forceableOp) {
             // Any declaration that can be forced.
             if (auto node = dyn_cast<NodeOp>(op))
@@ -103,7 +107,6 @@ class DiscoverLoops {
             recordDataflow(ref, data);
             recordProbe(data, ref);
           })
-          .Case<MemOp>([&](MemOp mem) { handleMemory(mem); })
           .Case<RefSendOp>([&](RefSendOp send) {
             recordDataflow(send.getResult(), send.getBase());
           })
@@ -165,33 +168,12 @@ class DiscoverLoops {
           .Case<FConnectLike>([&](FConnectLike connect) {
             recordDataflow(connect.getDest(), connect.getSrc());
           })
-          .Case<mlir::UnrealizedConversionCastOp>([&](auto) {
-            // Casts are cast-like regardless of source.
-            // UnrealizedConversionCastOp doesn't implement CastOpInterace,
-            // otherwise we would use it here.
-            for (auto res : op->getResults())
-              for (auto src : op->getOperands())
-                recordDataflow(res, src);
-          })
           .Default([&](Operation *op) {
-            // Non FIRRTL ops are not checked
-            if (!op->getDialect() ||
-                !isa<FIRRTLDialect, chirrtl::CHIRRTLDialect>(
-                    op->getDialect())) {
-              if (!foreignOps && op->getNumResults() > 0 &&
-                  op->getNumOperands() > 0) {
-                op->emitRemark("Non-firrtl operations detected, combinatorial "
-                               "loop checking may miss some loops.");
-                foreignOps = true;
-              }
-              return;
-            }
-            // All other expressions.
-            if (op->getNumResults() == 1) {
-              auto res = op->getResult(0);
+            // All other expressions are assumed to be combinational, so record
+            // the dataflow between all inputs to outputs.
+            for (auto res : op->getResults())
               for (auto src : op->getOperands())
                 recordDataflow(res, src);
-            }
           });
     });
   }
@@ -468,23 +450,6 @@ class DiscoverLoops {
     valToFieldRefs[result].emplace_back(base, fieldID);
   }
 
-  void handleMemory(MemOp mem) {
-    if (mem.getReadLatency() > 0)
-      return;
-    // Add the enable and address fields as the drivers of the data field.
-    for (auto memPort : mem.getResults())
-      // TODO: Can reftype ports create cycle ?
-      if (auto type = type_dyn_cast<BundleType>(memPort.getType())) {
-        auto enableFieldId = type.getFieldID((unsigned)ReadPortSubfield::en);
-        auto addressFieldId = type.getFieldID((unsigned)ReadPortSubfield::addr);
-        auto dataFieldId = type.getFieldID((unsigned)ReadPortSubfield::data);
-        addDrivenBy({memPort, static_cast<unsigned int>(dataFieldId)},
-                    {memPort, static_cast<unsigned int>(enableFieldId)});
-        addDrivenBy({memPort, static_cast<unsigned int>(dataFieldId)},
-                    {memPort, static_cast<unsigned int>(addressFieldId)});
-      }
-  }
-
   // Perform an iterative DFS traversal of the given graph. Record paths between
   // the ports and detect and report any cycles in the graph.
   LogicalResult dfsTraverse(const DrivenByGraphType &graph) {

test/Dialect/FIRRTL/check-comb-cycles.mlir

@@ -1036,46 +1036,6 @@ firrtl.circuit "UnrealizedConversionCast" {
 
 // -----
 
-firrtl.circuit "OutsideDialect" {
-  firrtl.module @OutsideDialect() {
-    // Other dialects might need to close loops in their own ops. Ignore 
-    // ops from other dialects
-    %b = firrtl.wire   : !firrtl.uint<32>
-    // expected-remark @below {{Non-firrtl operations detected, combinatorial loop checking may miss some loops.}}
-    %a = "foo"(%b) : (!firrtl.uint<32>) -> !firrtl.uint<32>
-    // Should only trigger once
-    %c = "foo"(%b) : (!firrtl.uint<32>) -> !firrtl.uint<32>
-    firrtl.matchingconnect %b, %a : !firrtl.uint<32>
-  }
-}
-
-// -----
-
-// Foreign op looks sink like, no loop
-firrtl.circuit "OutsideDialectSink" {
-  firrtl.module @OutsideDialectSink() {
-    // Other dialects might need to close loops in their own ops. Ignore 
-    // ops from other dialects
-    %b = firrtl.wire   : !firrtl.uint<32>
-    "foo"(%b) : (!firrtl.uint<32>) -> ()
-  }
-}
-
-// -----
-
-// Foreign op looks source like, no loop
-firrtl.circuit "OutsideDialectSource" {
-  firrtl.module @OutsideDialectSource() {
-    // Other dialects might need to close loops in their own ops. Ignore 
-    // ops from other dialects
-    %b = firrtl.wire   : !firrtl.uint<32>
-    %a = "foo"() : () -> !firrtl.uint<32>
-    firrtl.matchingconnect %b, %a : !firrtl.uint<32>
-  }
-}
-
-// -----
-
 // Check RWProbeOp + force doesn't crash.
 // No loop here.
 firrtl.circuit "Issue6820" {