[HW] Add pass to outline certain ops: hw-outline-ops #7991
Conversation
This pass outlines all hw::HWModuleOp operations into separate modules, uniquifying the modules. This is useful for debugging and cutting down RTL size for certain high level ops. Notes: - Does not support ops with regions. - In uniquifying the modules, dialect attributes are ignored. Only properties are observed. - Dialect attributes are, however, inherited by the module instances.
teqdruid
force-pushed
the
teqdruid/hw/outline
branch
from
34d67d3 to
9167f05
Compare
|
Tagging a few people as I don't know who's working this week. |
There was a problem hiding this comment.
Re. the region constriant: technically, it should be perfectly legal to outline operations with regions that are IsolatedFromAbove. I think this would be a great addition which would apply to e.g. the pipeline ops (which currently online has an inline lowering.
Regarding dialect attributes - i assume the reason why you didn't include this is because you can't generate a pretty (module) name for them. To expand capabilities, would it make sense to keep the existing name generation mechanism, but then additionally just hash the set of dialect/"other" attributes and append that to the module name?
If you opt not to do this, i think there should be some tests where the targeted ops have dialect attributes, to verify that everything works correctly.
In any case, this seems like a useful tool that may come in handy here or there when debugging RTL to force some more hierarchy into the trace.
|
|
||
| let options = [ | ||
| ListOption<"opNames", "op-names", "std::string", | ||
| "List of operation names to outline. If empty, this pass is a no-op."> |
There was a problem hiding this comment.
Just to foolproof this, show an example as well (to remove ambiguity about whether or not to include the dialect name (${dialect}.${opname}).
|
|
||
| // Append only the inherent attributes to the module name. | ||
| nameOS << "_attrs"; | ||
| if (auto regOp = op->getRegisteredInfo()) { |
There was a problem hiding this comment.
nit: A bit misleading to call this regOp - how about registeredOpName?
Problem with that is hashing and comparing regions would be expensive.
No. It's because dialect attributes like |
For how we use regions, yes. For the general MLIR notion, no. They could be all sorts of things (Exception landing pads?) which have semantics based on simply being in their parent, not based on name resolution of SSA values. |
| size for certain high level ops. | ||
|
|
||
| Notes: | ||
| - Does not support ops with regions. |
There was a problem hiding this comment.
I don't see how this is in general safe. There is no reason to believe a dialect attribute can be moved from a module to an instance. Take any lowering options or name hints, for example.
There was a problem hiding this comment.
Only they're not being moved from the module to the instance. They're being kept on the operation which is being replaced by an instance. So the dialect attributes aren't moving -- the underlying op is being moved into its own module. The name hints are the primary reason I did this.
| }; | ||
|
|
||
| /// Compare two OpInfos. | ||
| struct OpInfoEqual { |
There was a problem hiding this comment.
is this better than just specializing std::equal_to?
| }; | ||
| } // namespace | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
nit: member definitions don't need to be in a namespace.
| struct OpInfoEqual { | ||
| size_t operator()(const OpInfo &a, const OpInfo &b) const { | ||
| return a.name == b.name && a.operandTypes == b.operandTypes && | ||
| a.resultTypes == b.resultTypes && a.attributes == b.attributes; |
There was a problem hiding this comment.
On second thought, isn't this just the same as the default operator== that would be generated for the type and used by the default std::equal_to?
There was a problem hiding this comment.
I was unaware that any operation== was automatically generated.
| os << "_"; | ||
| std::string typeName; | ||
| llvm::raw_string_ostream(typeName) << type; | ||
| if (typeName[0] == '!') |
There was a problem hiding this comment.
getName()[0]?
Shouldn't need std::string
| }; | ||
|
|
||
| // Create the module name. | ||
| std::string name; |
There was a problem hiding this comment.
super nit: SmallString might be sizable to prevent allocation.
This pass outlines all of the selected operations into separate modules,
uniquifying the modules. This is useful for debugging and cutting down RTL
size for certain high level ops.
Notes:
properties are respected.