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] Improve LowerState to never produce read-after-write conflicts #7703

Merged
merged 1 commit into from
Oct 28, 2024
Merged

[Arc] Improve LowerState to never produce read-after-write conflicts #7703

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
2 changes: 0 additions & 2 deletions include/circt/Dialect/Arc/ArcPasses.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,9 @@ createInferMemoriesPass(const InferMemoriesOptions &options = {});
std::unique_ptr<mlir::Pass> createInlineArcsPass();
std::unique_ptr<mlir::Pass> createIsolateClocksPass();
std::unique_ptr<mlir::Pass> createLatencyRetimingPass();
std::unique_ptr<mlir::Pass> createLegalizeStateUpdatePass();
std::unique_ptr<mlir::Pass> createLowerArcsToFuncsPass();
std::unique_ptr<mlir::Pass> createLowerClocksToFuncsPass();
std::unique_ptr<mlir::Pass> createLowerLUTPass();
std::unique_ptr<mlir::Pass> createLowerStatePass();
std::unique_ptr<mlir::Pass> createLowerVectorizationsPass(
LowerVectorizationsModeEnum mode = LowerVectorizationsModeEnum::Full);
std::unique_ptr<mlir::Pass> createMakeTablesPass();
Expand Down
17 changes: 7 additions & 10 deletions include/circt/Dialect/Arc/ArcPasses.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,6 @@ def LatencyRetiming : Pass<"arc-latency-retiming", "mlir::ModuleOp"> {
];
}

def LegalizeStateUpdate : Pass<"arc-legalize-state-update", "mlir::ModuleOp"> {
let summary = "Insert temporaries such that state reads don't see writes";
let constructor = "circt::arc::createLegalizeStateUpdatePass()";
let dependentDialects = ["arc::ArcDialect"];
}

def LowerArcsToFuncs : Pass<"arc-lower-arcs-to-funcs", "mlir::ModuleOp"> {
let summary = "Lower arc definitions into functions";
let constructor = "circt::arc::createLowerArcsToFuncsPass()";
Expand All @@ -187,12 +181,15 @@ def LowerLUT : Pass<"arc-lower-lut", "arc::DefineOp"> {
let dependentDialects = ["hw::HWDialect", "comb::CombDialect"];
}

def LowerState : Pass<"arc-lower-state", "mlir::ModuleOp"> {
def LowerStatePass : Pass<"arc-lower-state", "mlir::ModuleOp"> {
let summary = "Split state into read and write ops grouped by clock tree";
let constructor = "circt::arc::createLowerStatePass()";
let dependentDialects = [
"arc::ArcDialect", "mlir::scf::SCFDialect", "mlir::func::FuncDialect",
"mlir::LLVM::LLVMDialect", "comb::CombDialect", "seq::SeqDialect"
"arc::ArcDialect",
"comb::CombDialect",
"mlir::LLVM::LLVMDialect",
"mlir::func::FuncDialect",
"mlir::scf::SCFDialect",
"seq::SeqDialect",
];
}

Expand Down
9 changes: 4 additions & 5 deletions integration_test/arcilator/JIT/dpi.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ func.func @add_mlir_impl(%arg0: i32, %arg1: i32, %arg2: !llvm.ptr) {
llvm.store %0, %arg2 : i32, !llvm.ptr
return
}

hw.module @arith(in %clock : i1, in %a : i32, in %b : i32, out c : i32, out d : i32) {
%seq_clk = seq.to_clock %clock

%0 = sim.func.dpi.call @add_mlir(%a, %b) clock %seq_clk : (i32, i32) -> i32
%1 = sim.func.dpi.call @mul_shared(%a, %b) clock %seq_clk : (i32, i32) -> i32
hw.output %0, %1 : i32, i32
}

func.func @main() {
%c2_i32 = arith.constant 2 : i32
%c3_i32 = arith.constant 3 : i32
Expand All @@ -34,18 +35,16 @@ func.func @main() {
arc.sim.instantiate @arith as %arg0 {
arc.sim.set_input %arg0, "a" = %c2_i32 : i32, !arc.sim.instance<@arith>
arc.sim.set_input %arg0, "b" = %c3_i32 : i32, !arc.sim.instance<@arith>
arc.sim.set_input %arg0, "clock" = %one : i1, !arc.sim.instance<@arith>

arc.sim.step %arg0 : !arc.sim.instance<@arith>
arc.sim.set_input %arg0, "clock" = %zero : i1, !arc.sim.instance<@arith>
arc.sim.step %arg0 : !arc.sim.instance<@arith>
%0 = arc.sim.get_port %arg0, "c" : i32, !arc.sim.instance<@arith>
%1 = arc.sim.get_port %arg0, "d" : i32, !arc.sim.instance<@arith>

arc.sim.emit "c", %0 : i32
arc.sim.emit "d", %1 : i32

arc.sim.step %arg0 : !arc.sim.instance<@arith>
arc.sim.set_input %arg0, "clock" = %one : i1, !arc.sim.instance<@arith>
arc.sim.step %arg0 : !arc.sim.instance<@arith>
%2 = arc.sim.get_port %arg0, "c" : i32, !arc.sim.instance<@arith>
%3 = arc.sim.get_port %arg0, "d" : i32, !arc.sim.instance<@arith>
arc.sim.emit "c", %2 : i32
Expand Down
1 change: 1 addition & 0 deletions integration_test/arcilator/JIT/initial-shift-reg.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ module {
%true = arith.constant 1 : i1

arc.sim.instantiate @shiftreg as %model {
arc.sim.step %model : !arc.sim.instance<@shiftreg>
arc.sim.set_input %model, "en" = %false : i1, !arc.sim.instance<@shiftreg>
arc.sim.set_input %model, "reset" = %false : i1, !arc.sim.instance<@shiftreg>
arc.sim.set_input %model, "din" = %ff : i8, !arc.sim.instance<@shiftreg>
Expand Down
3 changes: 2 additions & 1 deletion integration_test/arcilator/JIT/reg.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// RUN: arcilator %s --run --jit-entry=main | FileCheck %s
// REQUIRES: arcilator-jit

// CHECK: o1 = 2
// CHECK: o1 = 2
// CHECK-NEXT: o2 = 5
// CHECK-NEXT: o1 = 3
// CHECK-NEXT: o2 = 6
Expand Down Expand Up @@ -41,6 +41,7 @@ func.func @main() {
%step = arith.constant 1 : index

arc.sim.instantiate @counter as %model {
arc.sim.step %model : !arc.sim.instance<@counter>
Copy link
Member

Choose a reason for hiding this comment

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

Is this change required to run initialization?

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. In its current form, a call to the initial function only executes seq.initial blocks and initializes the storage for states. It does not propagate those changes to outputs and side-effecting ops though. I wasn't sure if the initial function should also call eval once after initialization (potentially triggering first state updates and other things to run immediately), or if the user might want more fine grained control over when the first eval happens. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I see that makes sense. I agree that we want to avoid implicitly calling eval from initial

%init_val1 = arc.sim.get_port %model, "o1" : i8, !arc.sim.instance<@counter>
%init_val2 = arc.sim.get_port %model, "o2" : i8, !arc.sim.instance<@counter>

Expand Down
5 changes: 3 additions & 2 deletions lib/Conversion/ConvertToArcs/ConvertToArcs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ using llvm::MapVector;

static bool isArcBreakingOp(Operation *op) {
return op->hasTrait<OpTrait::ConstantLike>() ||
isa<hw::InstanceOp, seq::CompRegOp, MemoryOp, ClockedOpInterface,
seq::InitialOp, seq::ClockGateOp, sim::DPICallOp>(op) ||
isa<hw::InstanceOp, seq::CompRegOp, MemoryOp, MemoryReadPortOp,
ClockedOpInterface, seq::InitialOp, seq::ClockGateOp,
sim::DPICallOp>(op) ||
op->getNumResults() > 1;
}

Expand Down
8 changes: 7 additions & 1 deletion lib/Dialect/Arc/ArcTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,17 @@ using namespace mlir;
#define GET_TYPEDEF_CLASSES
#include "circt/Dialect/Arc/ArcTypes.cpp.inc"

unsigned StateType::getBitWidth() { return hw::getBitWidth(getType()); }
unsigned StateType::getBitWidth() {
if (llvm::isa<seq::ClockType>(getType()))
return 1;
return hw::getBitWidth(getType());
}

LogicalResult
StateType::verify(llvm::function_ref<InFlightDiagnostic()> emitError,
Type innerType) {
if (llvm::isa<seq::ClockType>(innerType))
return success();
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary since it is now handled in getBitWidth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in hw::getBitWidth though 😢

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, you're calling hw::getBitWidth. Why not StateType::getBitWidth though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that is not available at this point 😢 (it takes the type from the this parameter instead of a function argument). I should just factor this out into a static helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me fix that post-merge.

if (hw::getBitWidth(innerType) < 0)
return emitError() << "state type must have a known bit width; got "
<< innerType;
Expand Down
4 changes: 2 additions & 2 deletions lib/Dialect/Arc/Transforms/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ add_circt_dialect_library(CIRCTArcTransforms
InlineArcs.cpp
IsolateClocks.cpp
LatencyRetiming.cpp
LegalizeStateUpdate.cpp
LowerArcsToFuncs.cpp
LowerClocksToFuncs.cpp
LowerLUT.cpp
LowerState.cpp
LowerStateRewrite.cpp
LowerVectorizations.cpp
MakeTables.cpp
MergeIfs.cpp
Expand All @@ -33,6 +32,7 @@ add_circt_dialect_library(CIRCTArcTransforms
CIRCTComb
CIRCTEmit
CIRCTHW
CIRCTLLHD
CIRCTOM
CIRCTSV
CIRCTSeq
Expand Down
Loading
Loading