Add cycle detection to ResourceSharingPass::PerformFoldingActions.

This change modifies the PerformFoldingActions function to check that the introduction of priority_sel input selection would not introduce a cycle when the folded nodes are merged together. This is to ensure the pass does not produce an unrecoverable error that would otherwise occur when the next pass attempts to iterate nodes using DfsVisitor.

This does not yet solve the underlying bug in folding action legalization or analysis that exists but surfaces that error in a more usable way.

PiperOrigin-RevId: 895433062

Author: angelomatni1

xls/passes/BUILD

@@ -1113,6 +1113,7 @@ xls_pass(
         "@com_google_absl//absl/random",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
+        "@com_google_absl//absl/strings:str_format",
         "@com_google_absl//absl/types:span",
         "@cppitertools",
     ],
@@ -4505,9 +4506,15 @@ cc_test(
     name = "resource_sharing_pass_test",
     srcs = ["resource_sharing_pass_test.cc"],
     deps = [
+        ":bdd_query_engine",
+        ":bit_provenance_analysis",
+        ":folding_graph",
+        ":node_dependency_analysis",
         ":optimization_pass",
         ":pass_base",
         ":resource_sharing_pass",
+        ":visibility_analysis",
+        ":visibility_expr_builder",
         "//xls/common:xls_gunit_main",
         "//xls/common/fuzzing:fuzztest",
         "//xls/common/status:matchers",
@@ -4527,7 +4534,9 @@ cc_test(
         "//xls/ir:source_location",
         "//xls/ir:value",
         "//xls/solvers:z3_ir_equivalence_testutils",
+        "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/log:check",
+        "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:status_matchers",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/time",

xls/passes/resource_sharing_pass.cc

@@ -37,6 +37,7 @@
 #include "absl/random/random.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
+#include "absl/strings/str_format.h"
 #include "absl/types/span.h"
 #include "cppitertools/combinations.hpp"
 #include "cppitertools/zip.hpp"
@@ -1700,6 +1701,7 @@ ResourceSharingPass::SelectFoldingActions(
 absl::StatusOr<bool> ResourceSharingPass::PerformFoldingActions(
     FunctionBase* f, int64_t next_node_id,
     VisibilityBuilder* visibility_builder,
+    const NodeBackwardDependencyAnalysis& nda,
     const std::vector<std::unique_ptr<NaryFoldingAction>>&
         folding_actions_to_perform) {
   bool modified = !folding_actions_to_perform.empty();
@@ -1917,6 +1919,26 @@ absl::StatusOr<bool> ResourceSharingPass::PerformFoldingActions(
     }
     XLS_RET_CHECK_EQ(new_operands.size(), 2);
 
+    // Ensure the priority selects do not depend on any of the folded nodes.
+    // This is to avoid creating a cycle and producing an unrecoverable error.
+    for (Node* new_operand : new_operands) {
+      auto make_error = [&](Node* folded_node, Node* select) {
+        return absl::InternalError(absl::StrFormat(
+            "Folding action input selection would create a cycle with node "
+            "because selection depends on it: %s -> ... -> %s; there is a bug "
+            "in the legalization of folding actions or in visibility analysis",
+            folded_node->ToString(), select->ToString()));
+      };
+      for (const auto& [from_node, _] : froms_to_use) {
+        if (nda.IsDependent(from_node, new_operand)) {
+          return make_error(from_node, new_operand);
+        }
+      }
+      if (nda.IsDependent(to_node, new_operand)) {
+        return make_error(to_node, new_operand);
+      }
+    }
+
     // - Step 2: Replace the operands of the @to_node to use the results of the
     //           new selectors computed at Step 1.
     VLOG(3) << "      Step 2: update the target of the folding transformation";
@@ -2090,8 +2112,9 @@ absl::StatusOr<bool> ResourceSharingPass::RunOnFunctionBaseInternal(
 
   // Perform the folding
   XLS_ASSIGN_OR_RETURN(
-      modified, PerformFoldingActions(f, next_node_id, &visibility_estimator,
-                                      folding_actions_to_perform));
+      modified,
+      PerformFoldingActions(f, next_node_id, &visibility_estimator,
+                            nda_backwards, folding_actions_to_perform));
 
   return modified;
 }

xls/passes/resource_sharing_pass.h

@@ -310,6 +310,7 @@ class ResourceSharingPass : public OptimizationFunctionBasePass {
   static absl::StatusOr<bool> PerformFoldingActions(
       FunctionBase* f, int64_t next_node_id,
       VisibilityBuilder* visibility_builder,
+      const NodeBackwardDependencyAnalysis& nda,
       const std::vector<std::unique_ptr<NaryFoldingAction>>&
           folding_actions_to_perform);
 

xls/passes/resource_sharing_pass_test.cc

@@ -20,7 +20,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "xls/common/fuzzing/fuzztest.h"
+#include "absl/container/flat_hash_set.h"
 #include "absl/log/check.h"
+#include "absl/status/status.h"
 #include "absl/status/status_matchers.h"
 #include "absl/status/statusor.h"
 #include "absl/time/time.h"
@@ -43,15 +45,23 @@
 #include "xls/ir/package.h"
 #include "xls/ir/source_location.h"
 #include "xls/ir/value.h"
+#include "xls/passes/bdd_query_engine.h"
+#include "xls/passes/bit_provenance_analysis.h"
+#include "xls/passes/folding_graph.h"
+#include "xls/passes/node_dependency_analysis.h"
 #include "xls/passes/optimization_pass.h"
 #include "xls/passes/pass_base.h"
+#include "xls/passes/visibility_analysis.h"
+#include "xls/passes/visibility_expr_builder.h"
 #include "xls/solvers/z3_ir_equivalence_testutils.h"
 
 namespace xls {
 
 namespace {
 
 using ::absl_testing::IsOkAndHolds;
+using ::absl_testing::StatusIs;
+using ::testing::HasSubstr;
 using ::xls::solvers::z3::ScopedVerifyEquivalence;
 
 class ResourceSharingPassTest : public IrTestBase {
@@ -1408,6 +1418,43 @@ TEST_F(ResourceSharingPassTest, PreventCyclesInFoldingChainsIndirect) {
   EXPECT_EQ(NumberOfShifts(f), 3);
 }
 
+using VisibilityEdges =
+    absl::flat_hash_set<OperandVisibilityAnalysis::OperandNode>;
+
+TEST_F(ResourceSharingPassTest, CatchesCyclesBeforeTransforming) {
+  auto p = CreatePackage();
+  FunctionBuilder fb(TestName(), p.get());
+  Type* u8 = p->GetBitsType(8);
+  BValue i = fb.Param("i", u8);
+  BValue X = fb.UMul(i, i, 8, SourceInfo(), "X");
+  BValue Y = fb.UMul(X, i, 8, SourceInfo(), "D");
+  XLS_ASSERT_OK_AND_ASSIGN(Function * f, fb.BuildWithReturnValue(Y));
+  int64_t next_node_id = 10;
+
+  BddQueryEngine bdd_engine;
+  XLS_ASSERT_OK(bdd_engine.Populate(f));
+  NodeForwardDependencyAnalysis nda;
+  XLS_ASSERT_OK(nda.Attach(f));
+  BitProvenanceAnalysis bpa;
+  XLS_ASSERT_OK(bpa.Populate(f));
+  VisibilityBuilder visibility_builder(next_node_id, &bdd_engine, nda, bpa);
+
+  std::vector<std::pair<Node*, VisibilityEdges>> from_X = {
+      std::make_pair(X.node(), VisibilityEdges{})};
+  auto fold_X_into_Y = std::make_unique<NaryFoldingAction>(
+      std::move(from_X), Y.node(), VisibilityEdges{});
+  std::vector<std::unique_ptr<NaryFoldingAction>> folding_actions_to_perform;
+  folding_actions_to_perform.push_back(std::move(fold_X_into_Y));
+
+  NodeBackwardDependencyAnalysis nda_backwards;
+  XLS_ASSERT_OK(nda_backwards.Attach(f));
+  EXPECT_THAT(
+      ResourceSharingPass::PerformFoldingActions(
+          f, next_node_id, &visibility_builder, nda_backwards,
+          folding_actions_to_perform),
+      StatusIs(absl::StatusCode::kInternal, HasSubstr("would create a cycle")));
+}
+
 }  // namespace
 
 }  // namespace xls