-
Notifications
You must be signed in to change notification settings - Fork 281
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
[Seq] Add clock gate operation #5082
Conversation
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.
LGTM!
@@ -24,6 +24,29 @@ using namespace mlir; | |||
using namespace circt; | |||
using namespace seq; | |||
|
|||
/// Determine the integer value of a constant operand. | |||
static std::optional<APInt> getConstantInt(Attribute operand) { |
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.
It feels like these are useful in general (and likely copied from combfolds). Would it make sense to put them in a shared utility header?
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.
I didn't find any such op in the comb folds yet. But we should definitely add such a header to reduce code duplication.
Add a `seq.clock_gate` operation that represents a safe, glitchless gating of a clock. It isn't totally clear to me which dialect this op best fits into, but given that it interacts with clocks and contains an internal storage element to avoid glitching, Seq seems like a good fit. This commit merely adds the op, canonicalizations, and basic tests. It isn't used or lowered yet. I imagine that the `LowerSeqToSV` pass would be able to lower this clock gate either to an extmodule or to an inline simulation model later.
c5a9b2a
to
eee4d86
Compare
OpFoldResult ClockGateOp::fold(FoldAdaptor adaptor) { | ||
// Forward the clock if one of the enables is always true. | ||
if (isConstantOne(adaptor.getEnable()) || | ||
isConstantOne(adaptor.getTestEnable())) |
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.
Does this need to check for existence of the enable first?
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.
That's done in isConstantOne
. The adaptor
will return null if the enable isn't there, and the function handles that case.
// CHECK-NEXT: %alwaysOff1 = hw.wire %false sym @alwaysOff1 | ||
// CHECK-NEXT: %alwaysOff2 = hw.wire %false sym @alwaysOff2 | ||
%1 = seq.clock_gate %clock, %false | ||
%2 = seq.clock_gate %clock, %false, %false |
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.
Using true for the enable would provide more coverage.
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.
There are separate always-on tests for the constant true case. The problem here is if I pass one of the enables as constant true, it will just make the clock gate fold away (the enables are ORed).
Actual clock gate cells have DTF signals eg a test enable override. Does this op include space for that...? (Did not try to read the code but curious your thoughts on this) |
Yes! The op has an optional |
Add a
seq.clock_gate
operation that represents a safe, glitchless gating of a clock. It isn't totally clear to me which dialect this op best fits into, but given that it interacts with clocks and contains an internal storage element to avoid glitching, Seq seems like a good fit.This commit merely adds the op, canonicalizations, and basic tests. It isn't used or lowered yet. I imagine that the
LowerSeqToSV
pass would be able to lower this clock gate either to an extmodule or to an inline simulation model later.