[Arc] Remove obsolete arc.clock_tree and arc.passthrough ops

The new LowerState pass does not produce `arc.clock_tree` and `arc.passthrough` ops anymore. Remove them from the dialect entirely.
llvm · Oct 14, 2024 · 2c1bc29 · 2c1bc29
1 parent e648462
commit 2c1bc29
Show file tree

Hide file tree

Showing 5 changed files with 17 additions and 153 deletions.
diff --git a/include/circt/Dialect/Arc/ArcOps.td b/include/circt/Dialect/Arc/ArcOps.td
@@ -450,21 +450,6 @@ class ClockTreeLikeOp<string mnemonic, list<Trait> traits = []>:
   let regions = (region SizedRegion<1>:$body);
 }
 
-def ClockTreeOp : ClockTreeLikeOp<"clock_tree"> {
-  let summary = "A clock tree";
-  let arguments = (ins I1:$clock);
-  let assemblyFormat = [{
-    $clock attr-dict-with-keyword $body
-  }];
-}
-
-def PassThroughOp : ClockTreeLikeOp<"passthrough"> {
-  let summary = "Clock-less logic that is on the pass-through path";
-  let assemblyFormat = [{
-    attr-dict-with-keyword $body
-  }];
-}
-
 def InitialOp : ClockTreeLikeOp<"initial"> {
   let summary = "Region to be executed at the start of simulation";
   let assemblyFormat = [{

diff --git a/lib/Dialect/Arc/Transforms/LowerClocksToFuncs.cpp b/lib/Dialect/Arc/Transforms/LowerClocksToFuncs.cpp
@@ -51,9 +51,6 @@ struct LowerClocksToFuncsPass
 
   Statistic numOpsCopied{this, "ops-copied", "Ops copied into clock trees"};
   Statistic numOpsMoved{this, "ops-moved", "Ops moved into clock trees"};
-
-private:
-  bool hasPassthroughOp;
 };
 } // namespace
 
@@ -71,33 +68,20 @@ LogicalResult LowerClocksToFuncsPass::lowerModel(ModelOp modelOp) {
   // Find the clocks to extract.
   SmallVector<InitialOp, 1> initialOps;
   SmallVector<FinalOp, 1> finalOps;
-  SmallVector<PassThroughOp, 1> passthroughOps;
   SmallVector<Operation *> clocks;
   modelOp.walk([&](Operation *op) {
     TypeSwitch<Operation *, void>(op)
-        .Case<ClockTreeOp>([&](auto) { clocks.push_back(op); })
         .Case<InitialOp>([&](auto initOp) {
           initialOps.push_back(initOp);
           clocks.push_back(initOp);
         })
         .Case<FinalOp>([&](auto op) {
           finalOps.push_back(op);
           clocks.push_back(op);
-        })
-        .Case<PassThroughOp>([&](auto ptOp) {
-          passthroughOps.push_back(ptOp);
-          clocks.push_back(ptOp);
         });
   });
-  hasPassthroughOp = !passthroughOps.empty();
 
   // Sanity check
-  if (passthroughOps.size() > 1) {
-    auto diag = modelOp.emitOpError()
-                << "containing multiple PassThroughOps cannot be lowered.";
-    for (auto ptOp : passthroughOps)
-      diag.attachNote(ptOp.getLoc()) << "Conflicting PassThroughOp:";
-  }
   if (initialOps.size() > 1) {
     auto diag = modelOp.emitOpError()
                 << "containing multiple InitialOps is currently unsupported.";
@@ -110,7 +94,7 @@ LogicalResult LowerClocksToFuncsPass::lowerModel(ModelOp modelOp) {
     for (auto op : finalOps)
       diag.attachNote(op.getLoc()) << "Conflicting FinalOp:";
   }
-  if (passthroughOps.size() > 1 || initialOps.size() > 1 || finalOps.size() > 1)
+  if (initialOps.size() > 1 || finalOps.size() > 1)
     return failure();
 
   // Perform the actual extraction.
@@ -126,7 +110,7 @@ LogicalResult LowerClocksToFuncsPass::lowerClock(Operation *clockOp,
                                                  Value modelStorageArg,
                                                  OpBuilder &funcBuilder) {
   LLVM_DEBUG(llvm::dbgs() << "- Lowering clock " << clockOp->getName() << "\n");
-  assert((isa<ClockTreeOp, PassThroughOp, InitialOp, FinalOp>(clockOp)));
+  assert((isa<InitialOp, FinalOp>(clockOp)));
 
   // Add a `StorageType` block argument to the clock's body block which we are
   // going to use to pass the storage pointer to the clock once it has been
@@ -148,14 +132,10 @@ LogicalResult LowerClocksToFuncsPass::lowerClock(Operation *clockOp,
   auto modelOp = clockOp->getParentOfType<ModelOp>();
   funcName.append(modelOp.getName());
 
-  if (isa<PassThroughOp>(clockOp))
-    funcName.append("_passthrough");
-  else if (isa<InitialOp>(clockOp))
+  if (isa<InitialOp>(clockOp))
     funcName.append("_initial");
   else if (isa<FinalOp>(clockOp))
     funcName.append("_final");
-  else
-    funcName.append("_clock");
 
   auto funcOp = funcBuilder.create<func::FuncOp>(
       clockOp->getLoc(), funcName,
@@ -167,17 +147,6 @@ LogicalResult LowerClocksToFuncsPass::lowerClock(Operation *clockOp,
   // Create a call to the function within the model.
   builder.setInsertionPoint(clockOp);
   TypeSwitch<Operation *, void>(clockOp)
-      .Case<ClockTreeOp>([&](auto treeOp) {
-        auto ifOp = builder.create<scf::IfOp>(clockOp->getLoc(),
-                                              treeOp.getClock(), false);
-        auto builder = ifOp.getThenBodyBuilder();
-        builder.template create<func::CallOp>(clockOp->getLoc(), funcOp,
-                                              ValueRange{modelStorageArg});
-      })
-      .Case<PassThroughOp>([&](auto) {
-        builder.template create<func::CallOp>(clockOp->getLoc(), funcOp,
-                                              ValueRange{modelStorageArg});
-      })
       .Case<InitialOp>([&](auto) {
         if (modelOp.getInitialFn().has_value())
           modelOp.emitWarning() << "Existing model initializer '"
@@ -197,16 +166,6 @@ LogicalResult LowerClocksToFuncsPass::lowerClock(Operation *clockOp,
   // Move the clock's body block to the function and remove the old clock op.
   funcOp.getBody().takeBody(clockRegion);
 
-  if (isa<InitialOp>(clockOp) && hasPassthroughOp) {
-    // Call PassThroughOp after init
-    builder.setInsertionPoint(funcOp.getBlocks().front().getTerminator());
-    funcName.clear();
-    funcName.append(modelOp.getName());
-    funcName.append("_passthrough");
-    builder.create<func::CallOp>(clockOp->getLoc(), funcName, TypeRange{},
-                                 ValueRange{funcOp.getBody().getArgument(0)});
-  }
-
   clockOp->erase();
   return success();
 }

diff --git a/test/Dialect/Arc/allocate-state.mlir b/test/Dialect/Arc/allocate-state.mlir
@@ -6,8 +6,8 @@ arc.model @test io !hw.modty<input x : i1, output y : i1> {
   // CHECK-NEXT: ([[PTR:%.+]]: !arc.storage<5780>):
 
   // CHECK-NEXT: arc.alloc_storage [[PTR]][0] : (!arc.storage<5780>) -> !arc.storage<1159>
-  // CHECK-NEXT: arc.passthrough {
-  arc.passthrough {
+  // CHECK-NEXT: arc.initial {
+  arc.initial {
     // CHECK-NEXT: [[SUBPTR:%.+]] = arc.storage.get [[PTR]][0] : !arc.storage<5780> -> !arc.storage<1159>
     %0 = arc.alloc_state %arg0 : (!arc.storage) -> !arc.state<i1>
     arc.alloc_state %arg0 : (!arc.storage) -> !arc.state<i8>
@@ -42,8 +42,8 @@ arc.model @test io !hw.modty<input x : i1, output y : i1> {
   // CHECK-NEXT: }
 
   // CHECK-NEXT: arc.alloc_storage [[PTR]][1168] : (!arc.storage<5780>) -> !arc.storage<4609>
-  // CHECK-NEXT: arc.passthrough {
-  arc.passthrough {
+  // CHECK-NEXT: arc.initial {
+  arc.initial {
     // CHECK-NEXT: [[SUBPTR:%.+]] = arc.storage.get [[PTR]][1168] : !arc.storage<5780> -> !arc.storage<4609>
     arc.alloc_memory %arg0 : (!arc.storage) -> !arc.memory<4 x i1, i1>
     arc.alloc_memory %arg0 : (!arc.storage) -> !arc.memory<4 x i8, i1>
@@ -69,8 +69,8 @@ arc.model @test io !hw.modty<input x : i1, output y : i1> {
   // CHECK-NEXT: }
 
   // CHECK-NEXT: arc.alloc_storage [[PTR]][5778] : (!arc.storage<5780>) -> !arc.storage<2>
-  // CHECK-NEXT: arc.passthrough {
-  arc.passthrough {
+  // CHECK-NEXT: arc.initial {
+  arc.initial {
     arc.root_input "x", %arg0 : (!arc.storage) -> !arc.state<i1>
     arc.root_output "y", %arg0 : (!arc.storage) -> !arc.state<i1>
     // CHECK-NEXT: [[SUBPTR:%.+]] = arc.storage.get [[PTR]][5778] : !arc.storage<5780> -> !arc.storage<2>

diff --git a/test/Dialect/Arc/lower-clocks-to-funcs-errors.mlir b/test/Dialect/Arc/lower-clocks-to-funcs-errors.mlir
@@ -6,7 +6,7 @@ arc.model @NonConstExternalValue io !hw.modty<> {
   // expected-note @+1 {{external value defined here:}}
   %0 = comb.add %c0_i9001, %c0_i9001 : i9001
   // expected-note @+1 {{clock tree:}}
-  arc.passthrough {
+  arc.initial {
     // expected-error @+2 {{operation in clock tree uses external value}}
     // expected-note @+1 {{clock trees can only use external constant values}}
     %1 = comb.sub %0, %0 : i9001
@@ -27,17 +27,12 @@ arc.model @ExistingInitializerAndFinalizer io !hw.modty<> initializer @Victim fi
 
 // -----
 
-// expected-error @below {{op containing multiple PassThroughOps cannot be lowered.}}
 // expected-error @below {{op containing multiple InitialOps is currently unsupported.}}
 // expected-error @below {{op containing multiple FinalOps is currently unsupported.}}
 arc.model @MultiInitAndPassThrough io !hw.modty<> {
 ^bb0(%arg0: !arc.storage<1>):
-  // expected-note @below {{Conflicting PassThroughOp:}}
-  arc.passthrough {}
   // expected-note @below {{Conflicting InitialOp:}}
   arc.initial {}
-  // expected-note @below {{Conflicting PassThroughOp:}}
-  arc.passthrough {}
   // expected-note @below {{Conflicting FinalOp:}}
   arc.final {}
   // expected-note @below {{Conflicting InitialOp:}}

diff --git a/test/Dialect/Arc/lower-clocks-to-funcs.mlir b/test/Dialect/Arc/lower-clocks-to-funcs.mlir
@@ -1,27 +1,14 @@
 // RUN: circt-opt %s --arc-lower-clocks-to-funcs --verify-diagnostics | FileCheck %s
 
-// CHECK-LABEL: func.func @Trivial_clock(%arg0: !arc.storage<42>) {
-// CHECK-NEXT:    [[TMP:%.+]] = hw.constant 9001
-// CHECK-NEXT:    call @DummyB([[TMP]]) {a}
-// CHECK-NEXT:    return
-// CHECK-NEXT:  }
-
-// CHECK-LABEL: func.func @Trivial_passthrough(%arg0: !arc.storage<42>) {
+// CHECK-LABEL: func.func @Trivial_initial(%arg0: !arc.storage<42>) {
 // CHECK-NEXT:    [[TMP:%.+]] = hw.constant 9002
 // CHECK-NEXT:    call @DummyB([[TMP]]) {b}
 // CHECK-NEXT:    return
 // CHECK-NEXT:  }
 
-// CHECK-LABEL: func.func @Trivial_initial(%arg0: !arc.storage<42>) {
+// CHECK-LABEL: func.func @Trivial_final(%arg0: !arc.storage<42>) {
 // CHECK-NEXT:    [[TMP:%.+]] = hw.constant 9003
 // CHECK-NEXT:    call @DummyB([[TMP]]) {c}
-// CHECK-NEXT:    call @Trivial_passthrough(%arg0)
-// CHECK-NEXT:    return
-// CHECK-NEXT:  }
-
-// CHECK-LABEL: func.func @Trivial_final(%arg0: !arc.storage<42>) {
-// CHECK-NEXT:    [[TMP:%.+]] = hw.constant 9004
-// CHECK-NEXT:    call @DummyB([[TMP]]) {d}
 // CHECK-NEXT:    return
 // CHECK-NEXT:  }
 
@@ -31,85 +18,23 @@
 // CHECK-SAME:    finalizer @Trivial_final
 // CHECK-SAME:  {
 // CHECK-NEXT:  ^bb0(%arg0: !arc.storage<42>):
-// CHECK-NEXT:    %true = hw.constant true
-// CHECK-NEXT:    scf.if %true {
-// CHECK-NEXT:      func.call @Trivial_clock(%arg0) : (!arc.storage<42>) -> ()
-// CHECK-NEXT:    }
-// CHECK-NEXT:    func.call @Trivial_passthrough(%arg0) : (!arc.storage<42>) -> ()
+// CHECK-NEXT:    [[TMP:%.+]] = hw.constant 9001
+// CHECK-NEXT:    call @DummyB([[TMP]]) {a}
 // CHECK-NEXT:  }
 
 arc.model @Trivial io !hw.modty<> {
 ^bb0(%arg0: !arc.storage<42>):
-  %true = hw.constant true
   %0 = hw.constant 9001 : i42
   %1 = hw.constant 9002 : i42
   %2 = hw.constant 9003 : i42
-  %3 = hw.constant 9004 : i42
-  arc.clock_tree %true {
-    func.call @DummyB(%0) {a} : (i42) -> ()
-  }
-  arc.passthrough {
-    func.call @DummyB(%1) {b} : (i42) -> ()
-  }
+  func.call @DummyB(%0) {a} : (i42) -> ()
   arc.initial {
-    func.call @DummyB(%2) {c} : (i42) -> ()
+    func.call @DummyB(%1) {b} : (i42) -> ()
   }
   arc.final {
-    func.call @DummyB(%3) {d} : (i42) -> ()
+    func.call @DummyB(%2) {c} : (i42) -> ()
   }
 }
 
 func.func private @DummyA() -> i42
 func.func private @DummyB(i42) -> ()
-
-//===----------------------------------------------------------------------===//
-
-// CHECK-LABEL: func.func @NestedRegions_passthrough(%arg0: !arc.storage<42>) {
-// CHECK-NEXT:    %true = hw.constant true
-// CHECK-NEXT:    scf.if %true {
-// CHECK-NEXT:      hw.constant 1337
-// CHECK-NEXT:    }
-// CHECK-NEXT:    return
-// CHECK-NEXT:  }
-
-// CHECK-LABEL: arc.model @NestedRegions io !hw.modty<> {
-// CHECK-NEXT:  ^bb0(%arg0: !arc.storage<42>):
-// CHECK-NEXT:    func.call @NestedRegions_passthrough(%arg0) : (!arc.storage<42>) -> ()
-// CHECK-NEXT:  }
-
-arc.model @NestedRegions io !hw.modty<> {
-^bb0(%arg0: !arc.storage<42>):
-  arc.passthrough {
-    %true = hw.constant true
-    scf.if %true {
-      %0 = hw.constant 1337 : i42
-    }
-  }
-}
-
-//===----------------------------------------------------------------------===//
-
-// The constants should copied to the top of the clock function body, not in
-// front of individual users, to prevent issues with caching and nested regions.
-// https://github.com/llvm/circt/pull/4685#discussion_r1132913165
-
-// CHECK-LABEL: func.func @InsertionOrderProblem_passthrough(%arg0: !arc.storage<42>) {
-// CHECK-NEXT:    %true = hw.constant true
-// CHECK-NEXT:    %false = hw.constant false
-// CHECK-NEXT:    scf.if %true {
-// CHECK-NEXT:      comb.add %true, %false
-// CHECK-NEXT:    }
-// CHECK-NEXT:    return
-// CHECK-NEXT:  }
-
-// CHECK-LABEL: arc.model @InsertionOrderProblem
-arc.model @InsertionOrderProblem io !hw.modty<> {
-^bb0(%arg0: !arc.storage<42>):
-  %true = hw.constant true
-  %false = hw.constant false
-  arc.passthrough {
-    scf.if %true {
-      comb.add %true, %false : i1
-    }
-  }
-}