-
Notifications
You must be signed in to change notification settings - Fork 277
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] Canonicalize transitive clock gates #5504
Conversation
This adds a folder for clock gates which removes clock gates that are transitively enable'd by other clock gates in a clock gate hierarchy.
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, with the small comment on test enable.
lib/Dialect/Seq/SeqOps.cpp
Outdated
// Transitive clock gating - eliminate clock gates that are driven by an | ||
// identical enable signal somewhere in a clock gate hierarchy. | ||
auto clockGateInput = getInput().getDefiningOp<ClockGateOp>(); | ||
while (clockGateInput) { | ||
if (clockGateInput.getEnable() == getEnable()) | ||
return getInput(); | ||
clockGateInput = clockGateInput.getInput().getDefiningOp<ClockGateOp>(); | ||
} |
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 think you also need to check the test_enable
here.
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.
hmm... would it then be correct to OR the test_enable
of the current clock gate with the TE of the upstream matching clock gate?
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.
Yeah I think you could also do that! Or just check if the test enables also agree, not just the regular enable. I'd expect in most designs there is no test enable, or if there is one, it's all connected to one single signal almost everywhere in the hierarchy. So you might not be losing much if you just check for it instead of creating a new comb.or 😃
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 think it's generally safe (from a power PoV) to remove the one further down the chain but am not completely sure.
lib/Dialect/Seq/SeqOps.cpp
Outdated
@@ -717,6 +717,15 @@ OpFoldResult ClockGateOp::fold(FoldAdaptor adaptor) { | |||
if (isConstantZero(adaptor.getInput())) | |||
return IntegerAttr::get(IntegerType::get(getContext(), 1), 0); | |||
|
|||
// Transitive clock gating - eliminate clock gates that are driven by an | |||
// identical enable signal somewhere in a clock gate hierarchy. |
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.
nit: "... somewhere higher in the clock..."
This adds a folder for clock gates which removes clock gates that are transitively enable'd by other clock gates in a clock gate hierarchy.
This adds a folder for clock gates which removes clock gates that are transitively enable'd by other clock gates in a clock gate hierarchy.