From 7da872ccf3fffebd3ccfc741284cd87f8927b57f Mon Sep 17 00:00:00 2001 From: Fabian Schuiki Date: Sun, 13 Oct 2024 19:35:10 -0700 Subject: [PATCH] [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. --- include/circt/Dialect/Arc/ArcOps.td | 15 ---- .../Arc/Transforms/LowerClocksToFuncs.cpp | 47 +--------- test/Dialect/Arc/allocate-state.mlir | 12 +-- .../Arc/lower-clocks-to-funcs-errors.mlir | 7 +- test/Dialect/Arc/lower-clocks-to-funcs.mlir | 89 ++----------------- 5 files changed, 17 insertions(+), 153 deletions(-) diff --git a/include/circt/Dialect/Arc/ArcOps.td b/include/circt/Dialect/Arc/ArcOps.td index addde7f02632..207423cc3214 100644 --- a/include/circt/Dialect/Arc/ArcOps.td +++ b/include/circt/Dialect/Arc/ArcOps.td @@ -450,21 +450,6 @@ class ClockTreeLikeOp 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 index 5c2fd10a7832..af318ef32557 100644 --- 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,11 +68,9 @@ LogicalResult LowerClocksToFuncsPass::lowerModel(ModelOp modelOp) { // Find the clocks to extract. SmallVector initialOps; SmallVector finalOps; - SmallVector passthroughOps; SmallVector clocks; modelOp.walk([&](Operation *op) { TypeSwitch(op) - .Case([&](auto) { clocks.push_back(op); }) .Case([&](auto initOp) { initialOps.push_back(initOp); clocks.push_back(initOp); @@ -83,21 +78,10 @@ LogicalResult LowerClocksToFuncsPass::lowerModel(ModelOp modelOp) { .Case([&](auto op) { finalOps.push_back(op); clocks.push_back(op); - }) - .Case([&](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(clockOp))); + assert((isa(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(); funcName.append(modelOp.getName()); - if (isa(clockOp)) - funcName.append("_passthrough"); - else if (isa(clockOp)) + if (isa(clockOp)) funcName.append("_initial"); else if (isa(clockOp)) funcName.append("_final"); - else - funcName.append("_clock"); auto funcOp = funcBuilder.create( 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(clockOp) - .Case([&](auto treeOp) { - auto ifOp = builder.create(clockOp->getLoc(), - treeOp.getClock(), false); - auto builder = ifOp.getThenBodyBuilder(); - builder.template create(clockOp->getLoc(), funcOp, - ValueRange{modelStorageArg}); - }) - .Case([&](auto) { - builder.template create(clockOp->getLoc(), funcOp, - ValueRange{modelStorageArg}); - }) .Case([&](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(clockOp) && hasPassthroughOp) { - // Call PassThroughOp after init - builder.setInsertionPoint(funcOp.getBlocks().front().getTerminator()); - funcName.clear(); - funcName.append(modelOp.getName()); - funcName.append("_passthrough"); - builder.create(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 index 72287f407050..ede5ee063289 100644 --- a/test/Dialect/Arc/allocate-state.mlir +++ b/test/Dialect/Arc/allocate-state.mlir @@ -6,8 +6,8 @@ arc.model @test io !hw.modty { // 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 arc.alloc_state %arg0 : (!arc.storage) -> !arc.state @@ -42,8 +42,8 @@ arc.model @test io !hw.modty { // 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 { // 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 arc.root_output "y", %arg0 : (!arc.storage) -> !arc.state // 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 index 94abfa1bb7ff..ae1286514ce3 100644 --- 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 index 22a954cffb32..8149542d9585 100644 --- 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 - } - } -}