[SV][HW] Stable port order in SVExtractTestCode

Change the BFS of operations used to compute backwards slices in SVExtractTestCode to, instead, use a DFS of operands. This ensures that any intermediary operations which may show up do not cause the ports of extracted modules to change. This was triggered by addition of the `seq.from_clock` operation to cause the BFS discovery order of roots to change. Signed-off-by: Schuyler Eldridge <[email protected]>
llvm · Sep 8, 2023 · 1698632 · 1698632
1 parent bf3abcd
commit 1698632
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 26 deletions.
diff --git a/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp b/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp
@@ -38,47 +38,48 @@ using BindTable = DenseMap<StringAttr, SmallDenseMap<StringAttr, sv::BindOp>>;
 //===----------------------------------------------------------------------===//
 
 // Reimplemented from SliceAnalysis to use a worklist rather than recursion and
-// non-insert ordered set.
+// non-insert ordered set.  Implement this as a DFS and not a BFS so that the
+// order is stable across changes to intermediary operations.  (It is then
+// necessary to use the _operands_ as a worklist and not the _operations_.)
 static void
 getBackwardSliceSimple(Operation *rootOp, SetVector<Operation *> &backwardSlice,
                        llvm::function_ref<bool(Operation *)> filter) {
-  SmallVector<Operation *> worklist;
-  worklist.push_back(rootOp);
+  SmallVector<Value> worklist(rootOp->getOperands());
 
   while (!worklist.empty()) {
-    Operation *op = worklist.back();
-    worklist.pop_back();
+    Value operand = worklist.pop_back_val();
+    Operation *definingOp = operand.getDefiningOp();
 
-    if (!op || op->hasTrait<mlir::OpTrait::IsIsolatedFromAbove>())
+    if (!definingOp ||
+        definingOp->hasTrait<mlir::OpTrait::IsIsolatedFromAbove>())
       continue;
 
     // Evaluate whether we should keep this def.
     // This is useful in particular to implement scoping; i.e. return the
     // transitive backwardSlice in the current scope.
-    if (filter && !filter(op))
+    if (filter && !filter(definingOp))
       continue;
 
-    for (auto en : llvm::enumerate(op->getOperands())) {
-      auto operand = en.value();
-      if (auto *definingOp = operand.getDefiningOp()) {
-        if (!backwardSlice.contains(definingOp))
-          worklist.push_back(definingOp);
-      } else if (auto blockArg = operand.dyn_cast<BlockArgument>()) {
-        Block *block = blockArg.getOwner();
-        Operation *parentOp = block->getParentOp();
-        // TODO: determine whether we want to recurse backward into the other
-        // blocks of parentOp, which are not technically backward unless they
-        // flow into us. For now, just bail.
-        assert(parentOp->getNumRegions() == 1 &&
-               parentOp->getRegion(0).getBlocks().size() == 1);
-        if (!backwardSlice.contains(parentOp))
-          worklist.push_back(parentOp);
-      } else {
-        llvm_unreachable("No definingOp and not a block argument.");
-      }
+    if (definingOp) {
+      if (!backwardSlice.contains(definingOp))
+        for (auto newOperand : llvm::reverse(definingOp->getOperands()))
+          worklist.push_back(newOperand);
+    } else if (auto blockArg = operand.dyn_cast<BlockArgument>()) {
+      Block *block = blockArg.getOwner();
+      Operation *parentOp = block->getParentOp();
+      // TODO: determine whether we want to recurse backward into the other
+      // blocks of parentOp, which are not technically backward unless they
+      // flow into us. For now, just bail.
+      assert(parentOp->getNumRegions() == 1 &&
+             parentOp->getRegion(0).getBlocks().size() == 1);
+      if (!backwardSlice.contains(parentOp))
+        for (auto newOperand : llvm::reverse(parentOp->getOperands()))
+          worklist.push_back(newOperand);
+    } else {
+      llvm_unreachable("No definingOp and not a block argument.");
     }
 
-    backwardSlice.insert(op);
+    backwardSlice.insert(definingOp);
   }
 }
 

diff --git a/test/Dialect/SV/hw-extract-test-code.mlir b/test/Dialect/SV/hw-extract-test-code.mlir
@@ -526,3 +526,29 @@ module {
   // CHECK-NEXT:  sv.bind <@Top::@__ETC_Assert_assert_1>
   // CHECK-NEXT:  sv.bind <@AssertWrapper::@__ETC_Assert_assert>
 }
+
+
+// -----
+
+// Check that the port order is [%clock, %in] and not the reverse.  This is
+// trying to guarantee taht the generated port order is stable with additions or
+// deletions of internal operations.  The critical thing in this test is the
+// existence of the `%0 = seq.from_clock %clock` operation.  This operation was
+// added and it caused the port order to become [%in, %clock].  This shouldn't
+// happen.
+//
+// See: https://github.com/llvm/circt/issues/6072
+
+module {
+  // CHECK-LABEL: hw.module @PortOrder_6072
+  hw.module @PortOrder_6072(%clock: !seq.clock, %in: i1) {
+    hw.instance "dut" @Foo(clock: %clock: !seq.clock, in: %in: i1) -> ()
+    hw.output
+  }
+  // CHECK: hw.module @Foo_cover(%clock: !seq.clock, %in: i1)
+  hw.module private @Foo(%clock: !seq.clock, %in: i1) {
+    %0 = seq.from_clock %clock
+    sv.cover.concurrent posedge %0, %in label "cover__hello"
+    hw.output
+  }
+}