diff --git a/lib/Dialect/FIRRTL/Transforms/CheckCombLoops.cpp b/lib/Dialect/FIRRTL/Transforms/CheckCombLoops.cpp index 639c6b65a362..9de5f5f3afd3 100644 --- a/lib/Dialect/FIRRTL/Transforms/CheckCombLoops.cpp +++ b/lib/Dialect/FIRRTL/Transforms/CheckCombLoops.cpp @@ -30,11 +30,9 @@ // and continue the analysis through the instance graph. //===----------------------------------------------------------------------===// -#include "circt/Dialect/FIRRTL/CHIRRTLDialect.h" #include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h" #include "circt/Dialect/FIRRTL/FIRRTLOps.h" #include "circt/Dialect/FIRRTL/FIRRTLUtils.h" -#include "circt/Dialect/FIRRTL/FIRRTLVisitors.h" #include "circt/Dialect/FIRRTL/Passes.h" #include "mlir/Pass/Pass.h" #include "llvm/ADT/DenseMap.h" @@ -65,6 +63,10 @@ class DiscoverLoops { /// drives the sink. using DrivenByGraphType = SmallVector>, 64>; + /// Given two nodes in the connectivity graph, record the operation that + /// drives the connectivity. This is only required for reporting and helps + /// debug combinational cycles. + using DriverOpMapType = DenseMap>; public: DiscoverLoops( @@ -72,7 +74,22 @@ class DiscoverLoops { const DenseMap &otherModulePortPaths, DrivenBysMapType &thisModulePortPaths) : module(module), instanceGraph(instanceGraph), - modulePortPaths(otherModulePortPaths), portPaths(thisModulePortPaths) {} + modulePortPaths(otherModulePortPaths), portPaths(thisModulePortPaths) { + // Default mode, still searching for existence of cycles. + reportPath = false; + } + + DiscoverLoops( + FModuleOp module, InstanceGraph &instanceGraph, + const DenseMap &otherModulePortPaths, + DrivenBysMapType &thisModulePortPaths, const FieldRef &dst, + const FieldRef &src) + : module(module), instanceGraph(instanceGraph), + modulePortPaths(otherModulePortPaths), portPaths(thisModulePortPaths), + srcInputPort(src), dstOutputPort(dst) { + // Combinational cycle already found, now report all the paths. + reportPath = true; + } LogicalResult processModule() { LLVM_DEBUG(llvm::dbgs() << "\n processing module :" << module.getName()); @@ -94,13 +111,15 @@ class DiscoverLoops { } walk(module, [&](Operation *op) { + // Record the op, to be used for reporting paths, once a cycle is + // detected. + visitingOp = op; llvm::TypeSwitch(op) .Case([&](hw::CombDataFlow df) { // computeDataFlow returns a pair of FieldRefs, first element is the // destination and the second is the source. for (auto [dest, source] : df.computeDataFlow()) addDrivenBy(dest, source); - }) .Case([&](Forceable forceableOp) { // Any declaration that can be forced. @@ -183,6 +202,7 @@ class DiscoverLoops { for (auto src : op->getOperands()) recordDataflow(res, src); }); + visitingOp = nullptr; }); } @@ -216,6 +236,7 @@ class DiscoverLoops { auto srcNode = getOrAddNode(src); auto dstNode = getOrAddNode(dst); drivenBy[dstNode].second.push_back(srcNode); + driverOpsMap[dstNode][srcNode] = visitingOp; } // Add `dstVal` as being driven by `srcVal`. @@ -394,7 +415,7 @@ class DiscoverLoops { // Check if sinkPort is a driver of sourcePort. auto drivenBysToSrcPort = modulePaths->second.find(modSrcPortField); if (drivenBysToSrcPort != modulePaths->second.end()) - if (llvm::find(drivenBysToSrcPort->second, modSinkPortField) != + if (drivenBysToSrcPort->second.find(modSinkPortField) != drivenBysToSrcPort->second.end()) { // This case can occur when there are multiple RWProbes on the // port, which refer to the same base value. So, each of such @@ -465,8 +486,8 @@ class DiscoverLoops { SmallVector onStack(numNodes, false); SmallVector dfsStack; - auto hasCycle = [&](unsigned rootNode, DenseSet &visited, - bool recordPortPaths = false) { + auto pathExists = [&](unsigned rootNode, DenseSet &visited, + bool recordPortPaths = false) { if (visited.contains(rootNode)) return success(); dfsStack.push_back(rootNode); @@ -504,18 +525,31 @@ class DiscoverLoops { } else if (onStack[neighbor]) { // Cycle found !! SmallVector path; - auto loopNode = neighbor; + auto beginPath = neighbor; + auto endPath = neighbor; + if (reportPath) { + // Report the path that exists between an output port and an input + // port. This maynot be a cycle. + beginPath = rootNode; + endPath = neighbor; + } // Construct the cyclic path. do { SmallVector::iterator it = - llvm::find_if(drivenBy[loopNode].second, + llvm::find_if(drivenBy[beginPath].second, [&](unsigned node) { return onStack[node]; }); - if (it == drivenBy[loopNode].second.end()) + if (it == drivenBy[beginPath].second.end()) break; - path.push_back(drivenBy[loopNode].first); - loopNode = *it; - } while (loopNode != neighbor); + path.push_back(drivenBy[beginPath].first); + beginPath = *it; + } while (beginPath != endPath); + if (reportPath) { + // The last element was not inserted, doesn't matter for reporting + // cycles, since it is same as the first one. But for paths, this + // is required. + path.push_back(drivenBy[beginPath].first); + } reportLoopFound(path, drivenBy[neighbor].first.getLoc()); return failure(); @@ -524,8 +558,19 @@ class DiscoverLoops { } return success(); }; - DenseSet visited; + if (reportPath) { + unsigned srcInputPortId = nodes[srcInputPort]; + onStack[srcInputPortId] = true; + // Insert the input port into the visited and onStack, this will ensure + // that the `hasCycle` fails, and reports the path from the output port to + // the input port. + visited.insert(srcInputPortId); + auto ret = pathExists(nodes[dstOutputPort], visited, false); + assert(ret.failed()); + return failure(); + } + for (unsigned node = 0; node < graph.size(); ++node) { bool isPort = false; if (auto arg = dyn_cast(drivenBy[node].first.getValue())) @@ -536,18 +581,24 @@ class DiscoverLoops { isPort = true; } - if (hasCycle(node, visited, isPort).failed()) + if (pathExists(node, visited, isPort).failed()) return failure(); } return success(); } void reportLoopFound(SmallVectorImpl &path, Location loc) { - auto errorDiag = mlir::emitError( - module.getLoc(), "detected combinational cycle in a FIRRTL module"); + auto errorDiag = + reportPath + ? mlir::emitRemark(module.getLoc(), "module involved in cycle") + : mlir::emitError( + module.getLoc(), + "detected combinational cycle in a FIRRTL module"); // Find a value we can name std::string firstName; FieldRef *it = llvm::find_if(path, [&](FieldRef v) { + if (v.getDefiningOp()) + return false; firstName = getName(v); return !firstName.empty(); }); @@ -569,10 +620,22 @@ class DiscoverLoops { bool lastWasDots = false; errorDiag << module.getName() << ".{" << getName(*it); + FieldRef prevField = *it; + using FieldPair = std::pair; + SmallVector portNodes; + const FieldPair empty = {{}, {}}; + FieldPair portPairs = empty; for (auto v : llvm::concat( llvm::make_range(std::next(it), path.end()), llvm::make_range(path.begin(), std::next(it)))) { auto name = getName(v); + unsigned dstNode = nodes[prevField]; + unsigned srcNode = nodes[v]; + if (driverOpsMap.contains(dstNode) && + driverOpsMap[dstNode].contains(srcNode)) { + Operation *o = driverOpsMap[dstNode][srcNode]; + errorDiag.attachNote(o->getLoc()) << "involved in cycle"; + } if (!name.empty()) { errorDiag << " <- " << name; lastWasDots = false; @@ -581,8 +644,45 @@ class DiscoverLoops { errorDiag << " <- ..."; lastWasDots = true; } + if (auto inst = v.getDefiningOp()) { + if (portPairs == empty) { + portPairs.first = v; + } else { + assert(portPairs.first.getDefiningOp() == inst); + portPairs.second = v; + portNodes.emplace_back(portPairs); + portPairs = empty; + } + } + prevField = v; } errorDiag << "}"; + while (!portNodes.empty()) { + auto portPair = portNodes.pop_back_val(); + auto dst = portPair.first; + auto src = portPair.second; + auto inst = dst.getDefiningOp(); + auto refMod = inst.getReferencedModule(instanceGraph); + if (!refMod) + continue; + + auto modulePaths = modulePortPaths.find(refMod); + if (modulePaths == modulePortPaths.end()) + continue; + auto map = modulePaths->getSecond(); + auto dstPortNum = cast(dst.getValue()).getResultNumber(); + auto srcPortNum = cast(src.getValue()).getResultNumber(); + auto refModDstPort = refMod.getArgument(dstPortNum); + auto refModSrcPort = refMod.getArgument(srcPortNum); + FieldRef dstPort(refModDstPort, dst.getFieldID()); + FieldRef srcPort(refModSrcPort, src.getFieldID()); + + DrivenBysMapType dummy; + DiscoverLoops rdf(refMod, instanceGraph, modulePortPaths, dummy, dstPort, + srcPort); + auto x = rdf.processModule(); + assert(x.failed()); + } } void dumpMap() { @@ -696,6 +796,10 @@ class DiscoverLoops { /// An eqv class of all the RWProbes that refer to the same base value. llvm::EquivalenceClasses rwProbeClasses; + DriverOpMapType driverOpsMap; + Operation *visitingOp = nullptr; + const FieldRef srcInputPort = {}, dstOutputPort = {}; + bool reportPath = false; }; /// This pass constructs a local graph for each module to detect diff --git a/test/Dialect/FIRRTL/check-comb-loops.mlir b/test/Dialect/FIRRTL/check-comb-loops.mlir index a36ef485bcb7..dabb02a04392 100644 --- a/test/Dialect/FIRRTL/check-comb-loops.mlir +++ b/test/Dialect/FIRRTL/check-comb-loops.mlir @@ -29,7 +29,9 @@ firrtl.circuit "hasloops" { %y = firrtl.wire : !firrtl.uint<1> %z = firrtl.wire : !firrtl.uint<1> firrtl.connect %c, %b : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %z, %y : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %y, %z : !firrtl.uint<1>, !firrtl.uint<1> firrtl.connect %d, %z : !firrtl.uint<1>, !firrtl.uint<1> } @@ -43,6 +45,7 @@ firrtl.circuit "loop" { // expected-error @below {{detected combinational cycle in a FIRRTL module, sample path: loop.{w <- w}}} firrtl.module @loop(out %y: !firrtl.uint<8>) { %w = firrtl.wire : !firrtl.uint<8> + // expected-note @below {{cycle}} firrtl.connect %w, %w : !firrtl.uint<8>, !firrtl.uint<8> } } @@ -56,8 +59,11 @@ firrtl.circuit "hasloops" { firrtl.module @hasloops(in %clk: !firrtl.clock, in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>, out %d: !firrtl.uint<1>) { %y = firrtl.wire : !firrtl.uint<1> firrtl.connect %c, %b : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} %t = firrtl.and %c, %y : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1> + // expected-note @below {{cycle}} %z = firrtl.node %t : !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %y, %z : !firrtl.uint<1>, !firrtl.uint<1> firrtl.connect %d, %z : !firrtl.uint<1>, !firrtl.uint<1> } @@ -73,16 +79,20 @@ firrtl.circuit "hasloops" { %y = firrtl.wire : !firrtl.uint<1> %z = firrtl.wire : !firrtl.uint<1> firrtl.connect %c, %b : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} %m_r = firrtl.mem Undefined {depth = 2 : i64, name = "m", portNames = ["r"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<1>> %0 = firrtl.subfield %m_r[clk] : !firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<1>> firrtl.connect %0, %clk : !firrtl.clock, !firrtl.clock %1 = firrtl.subfield %m_r[addr] : !firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<1>> + // expected-note @below {{cycle}} firrtl.connect %1, %y : !firrtl.uint<1>, !firrtl.uint<1> %2 = firrtl.subfield %m_r[en] : !firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<1>> %c1_ui = firrtl.constant 1 : !firrtl.uint firrtl.connect %2, %c1_ui : !firrtl.uint<1>, !firrtl.uint %3 = firrtl.subfield %m_r[data] : !firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<1>> + // expected-note @below {{cycle}} firrtl.connect %z, %3 : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %y, %z : !firrtl.uint<1>, !firrtl.uint<1> firrtl.connect %d, %z : !firrtl.uint<1>, !firrtl.uint<1> } @@ -94,7 +104,9 @@ firrtl.circuit "hasloops" { // Combination loop through an instance // CHECK-NOT: firrtl.circuit "hasloops" firrtl.circuit "hasloops" { + // expected-remark @below {{cycle}} firrtl.module @thru(in %in: !firrtl.uint<1>, out %out: !firrtl.uint<1>) { + // expected-note @below {{cycle}} firrtl.connect %out, %in : !firrtl.uint<1>, !firrtl.uint<1> } // expected-error @below {{detected combinational cycle in a FIRRTL module, sample path: hasloops.{inner.in <- y <- z <- inner.out <- inner.in}}} @@ -102,9 +114,13 @@ firrtl.circuit "hasloops" { %y = firrtl.wire : !firrtl.uint<1> %z = firrtl.wire : !firrtl.uint<1> firrtl.connect %c, %b : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} %inner_in, %inner_out = firrtl.instance inner @thru(in in: !firrtl.uint<1>, out out: !firrtl.uint<1>) + // expected-note @below {{cycle}} firrtl.connect %inner_in, %y : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %z, %inner_out : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %y, %z : !firrtl.uint<1>, !firrtl.uint<1> firrtl.connect %d, %z : !firrtl.uint<1>, !firrtl.uint<1> } @@ -124,11 +140,16 @@ firrtl.circuit "hasloops" { %e = firrtl.wire : !firrtl.uint<1> %0 = firrtl.and %c, %i : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1> firrtl.connect %a, %0 : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} %1 = firrtl.and %a, %d : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %b, %1 : !firrtl.uint<1>, !firrtl.uint<1> firrtl.connect %c, %b : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} %2 = firrtl.and %c, %e : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %d, %2 : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %e, %b : !firrtl.uint<1>, !firrtl.uint<1> firrtl.connect %o, %e : !firrtl.uint<1>, !firrtl.uint<1> } @@ -142,7 +163,9 @@ firrtl.circuit "matchingConnectAndConnect" { firrtl.module @matchingConnectAndConnect(out %a: !firrtl.uint<11>, out %b: !firrtl.uint<11>) { %w = firrtl.wire : !firrtl.uint<11> firrtl.matchingconnect %b, %w : !firrtl.uint<11> + // expected-note @below {{cycle}} firrtl.connect %a, %b : !firrtl.uint<11>, !firrtl.uint<11> + // expected-note @below {{cycle}} firrtl.matchingconnect %b, %a : !firrtl.uint<11> } } @@ -155,7 +178,9 @@ firrtl.circuit "outputPortCycle" { %0 = firrtl.subindex %reg[0] : !firrtl.vector>, 2> %1 = firrtl.subindex %reg[0] : !firrtl.vector>, 2> %w = firrtl.wire : !firrtl.bundle> + // expected-note @below {{cycle}} firrtl.connect %w, %0 : !firrtl.bundle>, !firrtl.bundle> + // expected-note @below {{cycle}} firrtl.connect %1, %w : !firrtl.bundle>, !firrtl.bundle> } } @@ -198,7 +223,9 @@ firrtl.circuit "PortReadWrite" { firrtl.module @PortReadWrite() { %a = firrtl.wire : !firrtl.uint<1> %bar_a = firrtl.instance bar interesting_name @Bar(in a: !firrtl.uint<1>) + // expected-note @below {{cycle}} firrtl.matchingconnect %bar_a, %a : !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.matchingconnect %a, %bar_a : !firrtl.uint<1> } } @@ -210,7 +237,9 @@ firrtl.circuit "Foo" { // expected-error @below {{Foo.{a <- bar.a <- a}}} firrtl.module @Foo(out %a: !firrtl.uint<1>) { %bar_a = firrtl.instance bar interesting_name @Bar(in a: !firrtl.uint<1>) + // expected-note @below {{cycle}} firrtl.matchingconnect %bar_a, %a : !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.matchingconnect %a, %bar_a : !firrtl.uint<1> } } @@ -224,7 +253,9 @@ firrtl.circuit "outputPortCycle" { %0 = firrtl.subindex %port[0] : !firrtl.vector, b: uint<4>>, 2> %1 = firrtl.subindex %port[0] : !firrtl.vector, b: uint<4>>, 2> %w = firrtl.instance bar interesting_name @Bar(in a: !firrtl.bundle, b: uint<4>>) + // expected-note @below {{cycle}} firrtl.connect %w, %0 : !firrtl.bundle, b: uint<4>>, !firrtl.bundle, b: uint<4>> + // expected-note @below {{cycle}} firrtl.connect %1, %w : !firrtl.bundle, b: uint<4>>, !firrtl.bundle, b: uint<4>> } } @@ -239,8 +270,11 @@ firrtl.circuit "hasloops" { %w = firrtl.wire : !firrtl.vector,10> %y = firrtl.subindex %w[3] : !firrtl.vector,10> firrtl.connect %c, %b : !firrtl.uint<1>, !firrtl.uint<1> + // expected-note @below {{cycle}} %0 = firrtl.and %c, %y : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1> + // expected-note @below {{cycle}} %z = firrtl.node %0 : !firrtl.uint<1> + // expected-note @below {{cycle}} firrtl.connect %y, %z : !firrtl.uint<1>, !firrtl.uint<1> firrtl.connect %d, %z : !firrtl.uint<1>, !firrtl.uint<1> } @@ -257,12 +291,15 @@ firrtl.circuit "hasloops" { %bar_b = firrtl.wire : !firrtl.vector, 2> %0 = firrtl.subindex %b[0] : !firrtl.vector, 2> %1 = firrtl.subindex %bar_a[0] : !firrtl.vector, 2> + // expected-note @below {{cycle}} firrtl.matchingconnect %1, %0 : !firrtl.uint<1> %4 = firrtl.subindex %bar_b[0] : !firrtl.vector, 2> %5 = firrtl.subindex %b[0] : !firrtl.vector, 2> + // expected-note @below {{cycle}} firrtl.matchingconnect %5, %4 : !firrtl.uint<1> %v0 = firrtl.subindex %bar_a[0] : !firrtl.vector, 2> %v1 = firrtl.subindex %bar_b[0] : !firrtl.vector, 2> + // expected-note @below {{cycle}} firrtl.matchingconnect %v1, %v0 : !firrtl.uint<1> } } @@ -274,18 +311,23 @@ firrtl.circuit "hasloops" { firrtl.circuit "hasLoops" { // expected-error @below {{hasLoops.{b[0] <- bar.b[0] <- bar.a[0] <- b[0]}}} firrtl.module @hasLoops(out %b: !firrtl.vector, 2>) { + // expected-note @below {{cycle}} %bar_a, %bar_b = firrtl.instance bar @Bar(in a: !firrtl.vector, 2>, out b: !firrtl.vector, 2>) %0 = firrtl.subindex %b[0] : !firrtl.vector, 2> %1 = firrtl.subindex %bar_a[0] : !firrtl.vector, 2> + // expected-note @below {{cycle}} firrtl.matchingconnect %1, %0 : !firrtl.uint<1> %4 = firrtl.subindex %bar_b[0] : !firrtl.vector, 2> %5 = firrtl.subindex %b[0] : !firrtl.vector, 2> + // expected-note @below {{cycle}} firrtl.matchingconnect %5, %4 : !firrtl.uint<1> } + // expected-remark @below {{cycle}} firrtl.module private @Bar(in %a: !firrtl.vector, 2>, out %b: !firrtl.vector, 2>) { %0 = firrtl.subindex %a[0] : !firrtl.vector, 2> %1 = firrtl.subindex %b[0] : !firrtl.vector, 2> + // expected-note @below {{cycle}} firrtl.matchingconnect %1, %0 : !firrtl.uint<1> %2 = firrtl.subindex %a[1] : !firrtl.vector, 2> %3 = firrtl.subindex %b[1] : !firrtl.vector, 2>