Improve pre-elaboration cycle detection error messages in XLS IR

xls/ir/BUILD

@@ -647,6 +647,7 @@ cc_library(
         "//xls/data_structures:leaf_type_tree",
         "@com_google_absl//absl/algorithm:container",
         "@com_google_absl//absl/base",
+        "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/base:no_destructor",
         "@com_google_absl//absl/container:btree",
         "@com_google_absl//absl/container:flat_hash_map",

xls/ir/function_base.h

@@ -28,6 +28,7 @@
 
 #include "absl/algorithm/container.h"
 #include "absl/base/no_destructor.h"
+#include "absl/base/optimization.h"
 #include "absl/container/btree_set.h"
 #include "absl/container/flat_hash_map.h"
 #include "absl/log/check.h"
@@ -178,6 +179,23 @@ class FunctionBase {
     kProc,
     kBlock,
   };
+  template <typename Sink>
+  friend void AbslStringify(Sink& sink, const Kind& k) {
+    switch (k) {
+      case Kind::kFunction:
+        absl::Format(&sink, "function");
+        return;
+      case Kind::kProc:
+        absl::Format(&sink, "proc");
+        return;
+      case Kind::kBlock:
+        absl::Format(&sink, "block");
+        return;
+    }
+    ABSL_UNREACHABLE();
+    absl::Format(&sink, "(unknown kind)");
+  }
+
   FunctionBase(std::string_view name, Package* package)
       : name_(name), package_(package) {}
   FunctionBase(const FunctionBase& other) = delete;

xls/ir/function_test.cc

@@ -405,6 +405,7 @@ TEST_F(FunctionTest, GraphWithCycle) {
 fn graph(p: bits[42], q: bits[42]) -> bits[42] {
   a: bits[42] = and(p, q)
   b: bits[42] = add(a, q)
+  d: bits[42] = add(b, q)
   ret c: bits[42] = sub(a, b)
 }
 )";
@@ -417,8 +418,26 @@ fn graph(p: bits[42], q: bits[42]) -> bits[42] {
         FindNode("a", f)->ReplaceOperand(FindNode("p", f), FindNode("b", f)));
     TestVisitor v;
     EXPECT_THAT(std::string(f->Accept(&v).message()),
-                HasSubstr(std::string("Cycle detected: [a -> b -> a]")));
-    EXPECT_DEATH(TopoSort(f), HasSubstr("Cycle detected: [a -> b -> a]"));
+                HasSubstr(std::string(
+                    "Cycle detected in function `graph`: [b -> a -> b]")));
+    EXPECT_DEATH(
+        TopoSort(f),
+        HasSubstr("Cycle detected in function `graph`: [b -> a -> b]"));
+  }
+  {
+    auto p = std::make_unique<Package>(TestName());
+    XLS_ASSERT_OK_AND_ASSIGN(Function * f, ParseFunction(input, p.get()));
+
+    // Introduce a cycle in the graph through three nodes.
+    ASSERT_TRUE(
+        FindNode("a", f)->ReplaceOperand(FindNode("p", f), FindNode("d", f)));
+    TestVisitor v;
+    EXPECT_THAT(std::string(f->Accept(&v).message()),
+                HasSubstr(std::string(
+                    "Cycle detected in function `graph`: [a -> b -> d -> a]")));
+    EXPECT_DEATH(
+        TopoSort(f),
+        HasSubstr("Cycle detected in function `graph`: [a -> b -> d -> a]"));
   }
 
   {
@@ -428,9 +447,11 @@ fn graph(p: bits[42], q: bits[42]) -> bits[42] {
     // Introduce a cycle in the graph through one node.
     XLS_ASSERT_OK(FindNode("a", f)->ReplaceOperandNumber(0, FindNode("a", f)));
     TestVisitor v;
-    EXPECT_THAT(std::string(f->Accept(&v).message()),
-                HasSubstr(std::string("Cycle detected: [a -> a]")));
-    EXPECT_DEATH(TopoSort(f), HasSubstr("Cycle detected: [a -> a]"));
+    EXPECT_THAT(
+        std::string(f->Accept(&v).message()),
+        HasSubstr(std::string("Cycle detected in function `graph`: [a -> a]")));
+    EXPECT_DEATH(TopoSort(f),
+                 HasSubstr("Cycle detected in function `graph`: [a -> a]"));
   }
 
   {
@@ -441,8 +462,11 @@ fn graph(p: bits[42], q: bits[42]) -> bits[42] {
     XLS_ASSERT_OK(FindNode("a", f)->ReplaceOperandNumber(1, FindNode("c", f)));
     TestVisitor v;
     EXPECT_THAT(std::string(f->Accept(&v).message()),
-                HasSubstr(std::string("Cycle detected: [a -> c -> a]")));
-    EXPECT_DEATH(TopoSort(f), HasSubstr("Cycle detected: [a -> c -> a]"));
+                HasSubstr(std::string(
+                    "Cycle detected in function `graph`: [a -> c -> a]")));
+    EXPECT_DEATH(
+        TopoSort(f),
+        HasSubstr("Cycle detected in function `graph`: [a -> c -> a]"));
   }
 }
 

xls/ir/node.cc

@@ -451,6 +451,43 @@ struct DfsStackFrame {
   decltype(std::declval<Node>().operands())::const_iterator operand_it;
 };
 
+namespace {
+absl::Status AnalyzeCycle(absl::Span<Node* const> cycle) {
+  std::vector<std::string> cycle_names;
+  cycle_names.reserve(cycle.size());
+  for (const Node* node : cycle) {
+    cycle_names.push_back(node->GetName());
+  }
+  std::string cycle_str = absl::StrJoin(cycle_names, " -> ");
+
+  if (cycle.empty()) {
+    return absl::InternalError("Empty cycle detected");
+  }
+
+  FunctionBase* fb = cycle.front()->function_base();
+  std::string message = absl::StrFormat("Cycle detected in %v `%s`: [%s]",
+                                        fb->kind(), fb->name(), cycle_str);
+
+  if (fb->IsBlock()) {
+    absl::StrAppend(
+        &message,
+        "\n\nIf this dependency cycle is intentional, you must insert a "
+        "register in the path to prevent combinational paths. Otherwise, if "
+        "this IR was generated (e.g., by XLS codegen), it is likely a bug in "
+        "the toolchain; please file a bug report.");
+  } else {
+    // Function or Proc
+    absl::StrAppend(&message,
+                    "\n\nIf this IR was generated (e.g., by the DSLX compiler "
+                    "or an optimization pass), it is likely a bug in the "
+                    "toolchain; please file a bug report. If you are "
+                    "constructing XLS IR directly, check for loops.");
+  }
+
+  return absl::InvalidArgumentError(message);
+}
+}  // namespace
+
 absl::Status Node::Accept(DfsVisitor* visitor) {
   if (visitor->IsVisited(this)) {
     return absl::OkStatus();
@@ -469,7 +506,7 @@ absl::Status Node::Accept(DfsVisitor* visitor) {
       }
       if (visitor->IsTraversing(operand)) {
         // Found a cycle, make a useful error message.
-        std::vector<std::string> cycle_names = {operand->GetName()};
+        std::vector<Node*> cycle_nodes = {operand};
         Node* node = operand;
         do {
           bool broke = false;
@@ -481,10 +518,12 @@ absl::Status Node::Accept(DfsVisitor* visitor) {
             }
           }
           CHECK(broke);
-          cycle_names.push_back(node->GetName());
+          cycle_nodes.push_back(node);
         } while (node != operand);
-        return absl::InternalError(absl::StrFormat(
-            "Cycle detected: [%s]", absl::StrJoin(cycle_names, " -> ")));
+        // The cycle is constructed backwards (target -> source), reverse it to
+        // be source -> target.
+        std::reverse(cycle_nodes.begin(), cycle_nodes.end());
+        return AnalyzeCycle(cycle_nodes);
       }
       saw_unvisited_operand = true;
       stack.push_back(DfsStackFrame{.node = operand,