-
Notifications
You must be signed in to change notification settings - Fork 312
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
[HW] Add passes to remove modules from the hierarchy and recursively expose IO of their instances #8070
base: main
Are you sure you want to change the base?
Conversation
1b1428a
to
10489b2
Compare
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 believe HWExpungeModule is basically ExtractInstance pass in FIRRTL, correct? If you are chisel user, probably you can use extractBlackBox annotation in the pass (https://github.com/llvm/circt/blob/main/lib/Dialect/FIRRTL/Transforms/ExtractInstances.cpp). That said I understand the motivation and I think generally it makes sense to implement something similar in HW as well, but I have a few thoughts on "portPrefix" option (this is basically FIRRTL non-local annotation implemented in pass option) and probably it will be necessary to discuss more.
// Reverse-topo order generated by module instances. | ||
// The topo sorted order does not change throught out the operation. It only | ||
// gets weakened, but still valid. |
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.
Please use llvm::post_order(instanceGraph)
for traversing the modules in post order.
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.
Changed in a6faaa7.
inst = newInst; | ||
} | ||
|
||
llvm::DenseMap<llvm::StringRef, mlir::Value> instOMap; |
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.
You should use StringMap
or llvm::DenseMap<StringAttr, ..>
. I think the current code contains UB.
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.
Fixed in a6faaa7. Thanks!
10489b2
to
a6faaa7
Compare
It does seem that way, with some slight differences. The implementation in this PR does not require an explicitly marked-out DUT, but in return, it lacks the functionality to place the extracted instances into a separate modules just under DUT. It also works in a per-module basis instead of an per-instance basis. However all these should be easy to change to be more inline with the FIRRTL pass.
Yeah that's currently implemented effectively as a janky One way to simplify this is to annotate a single instance/module (locally), without the complete path, and all newly created ports caused by this instance/module's moving up in its ancestors are given the same prefix. This should be sufficient if we only want to control the top-level's port names. Alternatively, if we want to keep this current behavior, maybe it's better to use annotations referencing |
a6faaa7
to
83e57c4
Compare
83e57c4
to
e00fc1f
Compare
This PR adds two passes:
hw-expunge-module
: Remove all instances of a list of specified modules, and recursively expose their ports as ports of parent modules.hw-tree-shake
: Except for listed ones and their (transitive) dependencies, remove all other HWModuleLikes within a mlir.module .The motivation was outlined in #8060. Basically, what we are trying to achieve is to split the entire RTL hierarchy into separate chunks, and place them onto multiple FPGAs. The original connections between chunks are now carried by inter-FPGA channels (e.g. Aurora). So basically these two passes enable us to convert the following hierarchy:
... into the following:
This is done by dropping all instances of expunged modules, and then recursively expose their ports as ports of (transitive) parents. The prefix given to generated ports can be specified explicitly, but no additional name collision detection is currently done. Instead, the pass relys on the automatic port renaming done by CIRCT.
The order in which expunged modules are processed does affect the result, so these modules are processed one by one.
TODO
hw.instance_choice
s are ignored in both passes right now. I'm not sure how to handle the instances where part of the choices are expunged, but not all of them.