-
Notifications
You must be signed in to change notification settings - Fork 309
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.
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
[RFC][Sim] Add triggered simulation procedures #7676
base: main
Are you sure you want to change the base?
Changes from all commits
6b2e806
2df8d10
90109c3
012faf2
0f307f9
6db4f06
d396282
42e2f59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
#ifndef CIRCT_DIALECT_SIM_SIMOPS_TD | ||
#define CIRCT_DIALECT_SIM_SIMOPS_TD | ||
|
||
include "circt/Dialect/HW/HWEnums.td" | ||
include "circt/Dialect/HW/HWOpInterfaces.td" | ||
include "circt/Dialect/HW/HWTypes.td" | ||
include "circt/Dialect/Seq/SeqTypes.td" | ||
|
@@ -21,7 +22,9 @@ include "circt/Dialect/Sim/SimTypes.td" | |
include "mlir/Interfaces/FunctionInterfaces.td" | ||
include "mlir/Interfaces/InferTypeOpInterface.td" | ||
include "mlir/Interfaces/SideEffectInterfaces.td" | ||
include "mlir/IR/BuiltinAttributes.td" | ||
include "mlir/IR/OpAsmInterface.td" | ||
include "mlir/IR/RegionKindInterface.td" | ||
|
||
class SimOp<string mnemonic, list<Trait> traits = []> : | ||
Op<SimDialect, mnemonic, traits>; | ||
|
@@ -359,4 +362,115 @@ def PrintFormattedProcOp : SimOp<"proc.print"> { | |
let assemblyFormat = "$input attr-dict"; | ||
} | ||
|
||
// --- Trigger Ops --- | ||
|
||
def OnEdgeOp : SimOp<"on_edge", [ | ||
Pure, | ||
DeclareOpInterfaceMethods<InferTypeOpInterface, ["inferReturnTypes"]> | ||
]> { | ||
let summary = "Create a trigger that gets invoked on a clock edge event."; | ||
let arguments = (ins ClockType:$clock, EventControlAttr:$event); | ||
let results = (outs EdgeTriggerType:$result); | ||
let assemblyFormat = "$event $clock attr-dict"; | ||
} | ||
|
||
def OnInitOp : SimOp<"on_init", [Pure]> { | ||
let summary = | ||
"Create a trigger that gets invoked at the start of simulation."; | ||
let results = (outs InitTriggerType:$result); | ||
let assemblyFormat = "attr-dict"; | ||
} | ||
|
||
def TriggerSequenceOp : SimOp<"trigger_sequence", [ | ||
DeclareOpInterfaceMethods<InferTypeOpInterface, ["inferReturnTypes"]> | ||
]> { | ||
let summary = "Derive a sequence of triggers from a parent trigger."; | ||
let description = [{ | ||
Creates a series of sequenced triggers. | ||
The first resulting trigger is invoked when the parent trigger is invoked. | ||
The subsequent triggers are invoked after all preceeding triggers | ||
have completed. The operation completes after all result triggers have | ||
completed. | ||
}]; | ||
let arguments = (ins AnyTriggerType:$parent, UI32Attr:$length); | ||
let results = (outs Variadic<AnyTriggerType>:$triggers); | ||
let assemblyFormat = | ||
"$parent `,` $length attr-dict `:` qualified(type($parent))"; | ||
let hasFolder = true; | ||
let hasCanonicalizeMethod = true; | ||
let hasVerifier = true; | ||
} | ||
|
||
def YieldSeqOp : SimOp<"yield_seq",[ | ||
Terminator, HasParent<"circt::sim::TriggeredOp"> | ||
]> { | ||
let summary = | ||
"Yield results from a triggered region with sequential semantics."; | ||
let description = [{ | ||
Terminates a triggered region and produces the given list of values. | ||
The results only become visible after all triggers and register updates | ||
occuring on the same event as the parent operation have completed. | ||
E.g., the following snippet produces a counter that increments on every | ||
rising edge of '%clk': | ||
``` | ||
%posedge = sim.on_edge posedge %clk | ||
%counter = sim.triggered (%counter) on %posedge tieoff [0 : i8] { | ||
^bb0(%arg0: i8): | ||
%cst1 = hw.constant 1 : i8 | ||
%inc = comb.add bin %arg0, %cst1 : i8 | ||
sim.yield_seq %inc : i8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yield significantly overlaps with registers. It makes sense to be able to get values out of triggered regions, but do we want trigger regions to have implicit storage?
performs the same thing without introducing redundant ways to store values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, it would make for tidier semantics. But I'm afraid the register-like behavior of
|
||
} : (i8) -> (i8) | ||
``` | ||
}]; | ||
let arguments = (ins Variadic<AnyType>:$inputs); | ||
let assemblyFormat = "($inputs^ `:` qualified(type($inputs)))? attr-dict"; | ||
let builders = [OpBuilder<(ins), "build($_builder, $_state, {});">]; | ||
} | ||
|
||
def TriggeredOp : SimOp<"triggered", [ | ||
IsolatedFromAbove, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wondering about why you decided to isolate it from above and pass through inputs explicitly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I don't see a hard technical reason to have it |
||
RegionKindInterface, | ||
RecursiveMemoryEffects, | ||
RecursivelySpeculatable, | ||
SingleBlockImplicitTerminator<"sim::YieldSeqOp">, | ||
HasParent<"circt::hw::HWModuleOp"> | ||
]> { | ||
let summary = | ||
"Defines a procedure invoked on a given trigger and condition."; | ||
let description = [{ | ||
Creates a procedural region which is invoked on a given trigger. | ||
The body region must complete without 'consuming' simulation time. It | ||
may not run indefinitely or wait for any simulation event. It is allowed to | ||
have side-effects and produce results. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does side-effect mean here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the million-dollar question. 😬
That requirement is both confusingly phrased and overly conservative. Maybe a better way of thinking about this is to separate between the hardware model (i.e., anything written in But I cannot say I've nailed this down, yet. |
||
For every result a 'tieoff' constant must be provided. It specifies the | ||
respective result's value before the body is first invoked. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how is a tie off different from an on_init trigger? This also seems to be a mechanism to make these sequences encode registers. Can we have a clean separation of state and triggers or do we really need to go down the verilog style inferred registers path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tie-offs provide a pre-initial value, i.e., the value an |
||
For non-simulation flows the results are replaced by their tie-off values. | ||
}]; | ||
let arguments = (ins AnyTriggerType:$trigger, | ||
Variadic<AnyType>:$inputs, | ||
OptionalAttr<TypedArrayAttrBase< | ||
TypedAttrInterface, "Tie-off constants">>:$tieoffs | ||
); | ||
let results = (outs Variadic<AnyType>); | ||
let regions = (region SizedRegion<1>:$body); | ||
|
||
let assemblyFormat = [{ | ||
` ` `(` $inputs `)` | ||
`on` ` ` `(` $trigger `:` qualified(type($trigger)) `)` | ||
(`tieoff` $tieoffs^)? attr-dict-with-keyword | ||
$body | ||
`:` functional-type($inputs, results) | ||
}]; | ||
|
||
let extraClassDeclaration = [{ | ||
// Implement RegionKindInterface. | ||
static RegionKind getRegionKind(unsigned index) { | ||
return RegionKind::SSACFG; | ||
} | ||
}]; | ||
|
||
let hasVerifier = true; | ||
let hasCanonicalizeMethod = true; | ||
} | ||
|
||
#endif // CIRCT_DIALECT_SIM_SIMOPS_TD |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,4 +26,19 @@ def FormatStringType : SimTypeDef<"FormatString"> { | |
}]; | ||
} | ||
|
||
|
||
def EdgeTriggerType : SimTypeDef<"EdgeTrigger"> { | ||
let summary = "Trigger derived from an edge event."; | ||
let parameters = (ins "::circt::hw::EventControl":$edgeEvent); | ||
let mnemonic = "trigger.edge"; | ||
let assemblyFormat = "`<` $edgeEvent `>`"; | ||
} | ||
|
||
def InitTriggerType : SimTypeDef<"InitTrigger"> { | ||
let summary = "Trigger derived from the simulation start event."; | ||
let mnemonic = "trigger.init"; | ||
} | ||
|
||
def AnyTriggerType : AnyTypeOf<[EdgeTriggerType, InitTriggerType]>; | ||
Comment on lines
+30
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need separate types for init and edge and why does edge need an event control attr? Why would a procedural region care by which trigger it was invoked? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was a provision for verilog lowering. The idea was to be able to determine whether we need to produce an |
||
|
||
#endif // CIRCT_DIALECT_SIM_SIMTYPES_TD |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
//===----------------------------------------------------------------------===// | ||
|
||
#include "circt/Dialect/Sim/SimDialect.h" | ||
#include "circt/Dialect/HW/HWDialect.h" | ||
#include "circt/Dialect/HW/HWOps.h" | ||
#include "circt/Dialect/Sim/SimOps.h" | ||
#include "mlir/IR/Builders.h" | ||
|
@@ -40,6 +41,12 @@ Operation *SimDialect::materializeConstant(::mlir::OpBuilder &builder, | |
::mlir::Type type, | ||
::mlir::Location loc) { | ||
|
||
// Delegate non 'sim' types to the HW dialect materializer. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious why this is necessary? Are hw constant requests really winding up here? Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is necessary materialize the tie-off integer attributes of TriggeredOps as I've just removed the fold method for the moment, as it relied on the |
||
if (!isa<SimDialect>(type.getDialect())) | ||
return builder.getContext() | ||
->getLoadedDialect<hw::HWDialect>() | ||
->materializeConstant(builder, value, type, loc); | ||
|
||
if (auto fmtStrType = llvm::dyn_cast<FormatStringType>(type)) | ||
return builder.create<FormatLitOp>(loc, fmtStrType, | ||
llvm::cast<StringAttr>(value)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -397,6 +397,198 @@ LogicalResult PrintFormattedProcOp::canonicalize(PrintFormattedProcOp op, | |
return failure(); | ||
} | ||
|
||
// --- OnEdgeOp --- | ||
|
||
LogicalResult OnEdgeOp::inferReturnTypes( | ||
MLIRContext *context, std::optional<Location> location, ValueRange operands, | ||
DictionaryAttr attributes, OpaqueProperties properties, RegionRange regions, | ||
SmallVectorImpl<Type> &inferredReturnTypes) { | ||
auto eventAttr = properties.as<OnEdgeOp::Properties *>()->getEvent(); | ||
inferredReturnTypes.emplace_back( | ||
EdgeTriggerType::get(context, eventAttr.getValue())); | ||
return success(); | ||
} | ||
|
||
// --- TriggeredOp --- | ||
|
||
LogicalResult TriggeredOp::verify() { | ||
if (getNumResults() > 0 && !getTieoffs()) | ||
return emitError("Tie-off constants must be provided for all results."); | ||
auto numTieoffs = !getTieoffs() ? 0 : getTieoffsAttr().size(); | ||
if (numTieoffs != getNumResults()) | ||
return emitError( | ||
"Number of tie-off constants does not match number of results."); | ||
if (numTieoffs == 0) | ||
return success(); | ||
unsigned idx = 0; | ||
bool failed = false; | ||
for (const auto &[res, tieoff] : | ||
llvm::zip(getResultTypes(), getTieoffsAttr())) { | ||
if (res != cast<TypedAttr>(tieoff).getType()) { | ||
emitError("Tie-off type does not match for result at index " + | ||
Twine(idx)); | ||
failed = true; | ||
} | ||
++idx; | ||
} | ||
return success(!failed); | ||
} | ||
|
||
LogicalResult TriggeredOp::canonicalize(TriggeredOp op, | ||
PatternRewriter &rewriter) { | ||
if (op.getNumResults() > 0) | ||
return failure(); | ||
|
||
auto *bodyBlock = &op.getBodyRegion().front(); | ||
if (bodyBlock->without_terminator().empty()) { | ||
rewriter.eraseOp(op); | ||
return success(); | ||
} | ||
|
||
return failure(); | ||
} | ||
|
||
// --- TriggerSequenceOp --- | ||
|
||
LogicalResult TriggerSequenceOp::inferReturnTypes( | ||
MLIRContext *context, std::optional<Location> location, ValueRange operands, | ||
DictionaryAttr attributes, OpaqueProperties properties, RegionRange regions, | ||
SmallVectorImpl<Type> &inferredReturnTypes) { | ||
// Create N results matching the type of the parent trigger, where N is the | ||
// specified length of the sequence. | ||
auto lengthAttr = | ||
properties.as<TriggerSequenceOp::Properties *>()->getLength(); | ||
uint32_t len = lengthAttr.getValue().getZExtValue(); | ||
Type trigType = operands.front().getType(); | ||
inferredReturnTypes.resize_for_overwrite(len); | ||
for (size_t i = 0; i < len; ++i) | ||
inferredReturnTypes[i] = trigType; | ||
return success(); | ||
} | ||
|
||
LogicalResult TriggerSequenceOp::verify() { | ||
if (getLength() != getNumResults()) | ||
return emitOpError("specified length does not match number of results."); | ||
return success(); | ||
} | ||
|
||
LogicalResult TriggerSequenceOp::fold(FoldAdaptor adaptor, | ||
SmallVectorImpl<OpFoldResult> &results) { | ||
// Fold trivial sequences to the parent trigger. | ||
if (getLength() == 1 && getResult(0) != getParent()) { | ||
results.push_back(getParent()); | ||
return success(); | ||
} | ||
return failure(); | ||
} | ||
|
||
LogicalResult TriggerSequenceOp::canonicalize(TriggerSequenceOp op, | ||
PatternRewriter &rewriter) { | ||
if (op.getNumResults() == 0) { | ||
rewriter.eraseOp(op); | ||
return success(); | ||
} | ||
|
||
// If the current op can be inlined into the parent, | ||
// leave it to the parent's canonicalization. | ||
if (auto parentSeq = op.getParent().getDefiningOp<TriggerSequenceOp>()) { | ||
if (parentSeq == op) { | ||
op.emitWarning("Recursive trigger sequence."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this a failure? Shouldn't it be covered by the verifier? What about multi-op sequences which are circular? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In practice a cyclic trigger graph is almost certainly a bug. But theoretically it is still well-formed: It has no root event, thus it is never invoked. There is no good way of handling this right now, but that should change with the soon to be added Multi-op circular graphs will be opportunistically handled by the flattening DFS. I'm not sure it is worth spending any effort on making sure all cycles are detected and replaced. |
||
return failure(); | ||
} | ||
if (op.getParent().hasOneUse()) | ||
return failure(); | ||
} | ||
|
||
auto getSingleSequenceUser = [](Value trigger) -> TriggerSequenceOp { | ||
if (!trigger.hasOneUse()) | ||
return {}; | ||
return dyn_cast<TriggerSequenceOp>(trigger.use_begin()->getOwner()); | ||
}; | ||
|
||
// Check if there are unused results (which can be removed) or | ||
// non-concurrent sub-sequences (which can be inlined). | ||
|
||
bool canBeChanged = false; | ||
for (auto res : op.getResults()) { | ||
auto singleSeqUser = getSingleSequenceUser(res); | ||
if (res.use_empty() || !!singleSeqUser) { | ||
canBeChanged = true; | ||
break; | ||
} | ||
} | ||
|
||
if (!canBeChanged) | ||
return failure(); | ||
|
||
// DFS for inlinable values. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be done instead on an op-by-op canonicalizer which doesn't walk the IR? having DFS in a canonicalizer is a good way to have n^2 behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is how I've implemented it originally, but I think the current variant is more efficient. Note that:
My motivation to replace the original implementation was to minimize the number of op rewrites. Since a TriggerSequenceOp can have a lot of results and users, I didn't want to substitute them repeatedly. The DFS makes sure this happens in one shot. Given all this, I do not see how this would cause a performance regression. But maybe I'm missing something? |
||
SmallVector<Value> newResultValues; | ||
SmallVector<TriggerSequenceOp> inlinedSequences; | ||
llvm::SmallVector<std::pair<TriggerSequenceOp, unsigned>> sequenceOpStack; | ||
|
||
sequenceOpStack.push_back({op, 0}); | ||
while (!sequenceOpStack.empty()) { | ||
auto &top = sequenceOpStack.back(); | ||
auto currentSequence = top.first; | ||
unsigned resultIndex = top.second; | ||
|
||
while (resultIndex < currentSequence.getNumResults()) { | ||
auto currentResult = currentSequence.getResult(resultIndex); | ||
// Check we do not walk in a cycle. | ||
if (currentResult == op.getParent()) { | ||
op.emitWarning("Recursive trigger sequence."); | ||
return failure(); | ||
} | ||
|
||
if (auto inlinableChildSequence = getSingleSequenceUser(currentResult)) { | ||
// Save the next result index to visit on the | ||
// stack and put the new sequence on top. | ||
top.second = resultIndex + 1; | ||
sequenceOpStack.push_back({inlinableChildSequence, 0}); | ||
inlinedSequences.push_back(inlinableChildSequence); | ||
inlinableChildSequence->dropAllReferences(); | ||
break; | ||
} | ||
|
||
if (!currentResult.use_empty()) | ||
newResultValues.push_back(currentResult); | ||
resultIndex++; | ||
} | ||
// Pop the sequence off of the stack if we have visited all results. | ||
if (resultIndex >= currentSequence.getNumResults()) | ||
sequenceOpStack.pop_back(); | ||
} | ||
|
||
// Remove dead sequences. | ||
if (newResultValues.empty()) { | ||
for (auto deadSubSequence : inlinedSequences) | ||
rewriter.eraseOp(deadSubSequence); | ||
rewriter.eraseOp(op); | ||
return success(); | ||
} | ||
|
||
// Replace the current operation with a new sequence. | ||
rewriter.setInsertionPoint(op); | ||
|
||
SmallVector<Location> inlinedLocs; | ||
inlinedLocs.reserve(inlinedSequences.size() + 1); | ||
inlinedLocs.push_back(op.getLoc()); | ||
for (auto subSequence : inlinedSequences) | ||
inlinedLocs.push_back(subSequence.getLoc()); | ||
auto fusedLoc = FusedLoc::get(op.getContext(), inlinedLocs); | ||
inlinedLocs.clear(); | ||
|
||
auto newOp = rewriter.create<TriggerSequenceOp>(fusedLoc, op.getParent(), | ||
newResultValues.size()); | ||
for (auto [rval, newRes] : llvm::zip(newResultValues, newOp.getResults())) | ||
rewriter.replaceAllUsesWith(rval, newRes); | ||
|
||
for (auto deadSubSequence : inlinedSequences) | ||
rewriter.eraseOp(deadSubSequence); | ||
rewriter.eraseOp(op); | ||
return success(); | ||
} | ||
|
||
//===----------------------------------------------------------------------===// | ||
// TableGen generated logic. | ||
//===----------------------------------------------------------------------===// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does register updates mean here? Registers in seq are not tied to these triggers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relates to what I've tried to formulate in my comment above:
That point is crucial to ensure we don't get any race conditions when mixing registers and the results of TriggeredOps. In practice this means for a a legal SV lowering the results must be produced via a non-blocking assignment that is evaluated at the same timestep and in the same scheduling region as registers updated on the clock that the given trigger is sensitive to.