-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Mark Certain LazyModules For Inlining #2579
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hcook
force-pushed
the
lazymodule-passthru
branch
from
July 28, 2020 20:54
e9fc4d4
to
88a22cc
Compare
sequencer
reviewed
Jul 28, 2020
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.
Reviewed diplomacy package, I have no knowledge to tilelink package, leave that to others. :)
hcook
force-pushed
the
lazymodule-passthru
branch
from
July 29, 2020 21:38
45e3e22
to
bc63143
Compare
terpstra
approved these changes
Jul 29, 2020
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 seems like a great idea.
terpstra
reviewed
Jul 29, 2020
terpstra
reviewed
Jul 29, 2020
terpstra
reviewed
Jul 29, 2020
terpstra
reviewed
Jul 29, 2020
terpstra
reviewed
Jul 29, 2020
hcook
force-pushed
the
lazymodule-passthru
branch
from
July 31, 2020 23:18
91907ff
to
34c4643
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a number of situations where we insert a node in a diplomatic graph, but under certain edge cardinalities or parameterizations the resulting module only connects inputs to outputs, i.e. becomes "passthrough". This PR provides infrastructure to:
BaseNode
will have no effect on the generated module circuitry (circuitIdentity
)LazyModule
'smodule
implementation should be inlined by FIRRTL (shouldBeInlined
)module
to convey this intent to FIRRTLidentity
andshouldBeInlined
flags in thegraphml
output (as empty ellipses and dotted-line rectangles respectively).The goal with this top-down approach is to explicitly capture designer intent for when certain LazyModules should be elided (in contrast to past bottom-up attempts to infer it by lifting all input-output connections). For example, this approach has the beneficial side effect of only inlining modules that contain nodes for which we already know we do not insert Monitors. Chisel modules like
OptimizationBarrier
are unaffected.The policy by which the flags are set remains open to the instantiater. The generic inlining policy for all
LazyModules
is now:This policy allows for recursive inlining of nested wrappers. The question becomes, which nodes are marked as
circuitIdentity
, and/or when do we overrideshouldBeInlined
directly? Some node types (IdentityNode, NameNode, EphemeralNode
) are definitionallycircuitIdentity
based on their kind. But for other types of nodes, I was less comfortable claiming that just because the node might e.g. appear to do nothing to the parameters, the matching circuit implementation is guaranteed to be desirable to inline.Currently I am trying three different approaches in three different use cases:
circuitIdentity
for certain specificTLAdapterNodes
,TLNexusNodes
andTLJunctionNodes
. For the adapter cases, there is an node-specific function on the edge parameters (possibly when combined with an independent sizing parameter) that results in the adapter being passthrough. For theNexusNodes
andJunctionNodes
circuitIdentity
often occurs when there is only a single input and output, or a ratio of 1. This cleans up a lot of confusingly named "xbars" that happen to frequently have 1:1 connections.shouldBeInlined
for certainTLBusWrappers
. These usually contain a child with aNexusNode
that might becircuitIdentity
, but also aClockSinkNode
that never is (but serves no function in the nexuscircuitIdentity
case).While it is tempting to set
circuitIdentity
for certain node types based purely on their inward/outward down/up parameters (i.e. cases where the parameters on both side of an AdapterNode are identical) I wasn't comfortable making that the default behavior. One could always write a circuit generator that modifies the data flowing through a node while not capturing the change in diplomatic parameters; but is doing so an anti-pattern that should require explicit control to prevent inlining? Even if so, it seems safer to having thecircuitIdentity
identification handled on a per-node basis. Some modules likeTLFilter
only change the parameters but not the actual circuit.Some code generation results with SiFive datapoints: 8-20% reduction in the number of Module classes, 3-6% reduction in lines of verilog emitted. Obviously these modules were "empty" and the actual impact on final area has yet to be evaluated, but it can't hurt to spend less time processing this worthless connectivity in downstream tools.