Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: fschuiki/arc-new-lower-state
Choose a base branch
from
+17 −153
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions include/circt/Dialect/Arc/ArcOps.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [{
Expand Down
47 changes: 3 additions & 44 deletions lib/Dialect/Arc/Transforms/LowerClocksToFuncs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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.";
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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 '"
Expand All @@ -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)});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the initial does affect an output port value directly, do we not need to guarantee that this change is propagated to the output port?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good question. Since passthrough is now gone, this boils down to whether the initial function should also call the eval function to propagate the effects of the initial assignments to outputs and side-effecting ops. I'm trying to think whether there is a use case where the user wants to run initial, but then do some other tweaking (or maybe dump a first timestep to VCD?) before calling the first eval and potentially triggering output or state changes.

@uenoku Has observed that some simulators have fairly intricate bring-up sequences. Generally it seems like there's an initial value assignment to variables, often X, then an execution of all variable initializers, then executing all initial blocks, and then triggering any posedge and the like based on those variables. We might need to do something similar at some point? Maybe some form of tiered arc.initial to have containers for the different rounds of initialization?

Copy link
Contributor

@fzi-hielscher fzi-hielscher Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I've been desperately trying to avoid so far. But as the SV LRM so nonchalantly states on initialization at declaration:

This may require a special pre-initial pass at run time.

☹️

Considering that commercial simulators also tend to take their freedoms here [citation needed], I was hoping we could avoid that madness. I feel when we have to debate whether a posedge has occurred when a bit is default initialized to 0, then initialized to 1 at declaration, and then initialized back to 0 in an initial process, we have already lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I totally agree. We should probably imitate the basic fixing of Verilog semantics that simulators do with their observable X-to-initial-value transitions, because otherwise Verilog's always @(...) combinational processes and async resets are virtually unusable. But we should do as little as possible/necessary.

Copy link
Contributor

@fzi-hielscher fzi-hielscher Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the triggered processes over in #7676 I currently require a tieoff constant for all processes, which also serves as pre-initial value. I've thought about changing this to take !seq.immutable values. Considering that neither me nor @uenoku have intended it that way, I think this would mesh surprisingly well. It would de facto push seq.initial into the pre-initial phase and we wouldn't be able to use non-!seq.immutable values in that phase. But I guess this is what we already arrived at in #7638 ?!?

But to go back to Martin's original question here: I think it is a fair choice to not force propagation after initialization. In the current implementation I've ended up introducing an inconsistency between models without an initializer (which won't call passthrough at startup) vs. the same model with an empty initializer (which will call passthrough). It's a good thing to avoid that.


clockOp->erase();
return success();
}
Expand Down
12 changes: 6 additions & 6 deletions test/Dialect/Arc/allocate-state.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand All @@ -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>
Expand Down
7 changes: 1 addition & 6 deletions test/Dialect/Arc/lower-clocks-to-funcs-errors.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:}}
Expand Down
89 changes: 7 additions & 82 deletions test/Dialect/Arc/lower-clocks-to-funcs.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: }

Expand All @@ -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
}
}
}
Loading