[FIRRTL] Add InstanceInfo Analysis #7612
Conversation
Add a new InstanceInfo analysis. This analysis provides common information which can be computed from a single walk of the InstanceGraph. The idea behind this is that there are a large amount of common information that different passes want to compute which all follows this same walk (and therefore has the same algorithmic complexity to compute it). Move all this information into a single, common analysis. This analysis currently enables O(1) queries of if something is the DUT, is instantiated under the DUT, or is instantiated under a layer. More queries can be added as they are identified. This new analysis naturally depends on the InstanceGraph analysis. Signed-off-by: Schuyler Eldridge <[email protected]>
| }; | ||
| } // namespace | ||
|
|
||
| InstanceInfo::InstanceInfo(Operation *op, mlir::AnalysisManager &am) { |
There was a problem hiding this comment.
Is the op required here ?
There was a problem hiding this comment.
According to the docs [1], yes, it needs to have a constructor of (Operation *) or (Operation *, AnalysisManager &). I tested this just now and the compiler will reject it without it.
Good eye, though. This is clearly unused in the body.
| bool isDut; | ||
|
|
||
| /// Indicates if this module is instantiated under the design-under-test. | ||
| InstanceInfo::LatticeValue underDut = {LatticeValue::Kind::Unknown}; |
There was a problem hiding this comment.
Just curious, not sure if this can be neatly reflected here or not but is the DUT module itself considered "underDUT"? ancestor / proper ancestor sort of thing?
There was a problem hiding this comment.
I think this interpretation of underDut makes more sense.
There was a problem hiding this comment.
I switched to some very exact language in 09b03f1. This language could hint the new interpretation of "under" isn't quite right. Specifically, is an instance of the DUT under the DUT? I went back and forth here and settled on it is under the DUT, i.e., the new interpretation makes sense.
Changed in 0d28c7a.
WDYT?
There was a problem hiding this comment.
After further thought on this, I changed to use "any" instead of "atLeastOne" in fa3de1e. This tends to better match, for lack of better inspiration, functional programming style "any of" or "all of" methods. "any" is also much shorter.
lib/Analysis/FIRRTLInstanceInfo.cpp
Outdated
|
|
||
| bool InstanceInfo::isUnderDut(FModuleOp op) { | ||
| auto underDut = getModuleAttributes(op).underDut; | ||
| return underDut.kind == LatticeValue::Mixed || |
There was a problem hiding this comment.
Especially if this grows more properties, some helpers on the lattice value to interpret it (like we do with other LatticeValue's in our code base) might help reduce boilerplate and chance of accidental mistakes / improve readability.
isPossiblyTrue() (kind >= Constant) /isKnownTrue() (kind == Constant && constant) or even may/must, just as ideas.
This is fine as-is for now.
There was a problem hiding this comment.
Yeah, this should grow some of these helpers. After applying a comment from Prithayan to reuse the allInstances* functions, the need for these helpers is lessened in this PR.
I'll leave this to future work.
There was a problem hiding this comment.
There are some helpers inspired from what IMCP's lattice value provides in 7efde85.
lib/Analysis/FIRRTLInstanceInfo.cpp
Outdated
|
|
||
| bool InstanceInfo::isDut(FModuleOp op) { return getModuleAttributes(op).isDut; } | ||
|
|
||
| bool InstanceInfo::isUnderDut(FModuleOp op) { |
There was a problem hiding this comment.
This seems more like, a "May analysis", so mayBeUnderDut is an option unless the difference between isUnderDut and isFullyUnderDut is well understood.
There was a problem hiding this comment.
I'd opt against "maybe/may be" only because it implies the possibility of something being true. I.e., this seems closer to "unknown". Since I couldn't think of something pithy, I opted for more verbose names in 09b03f1. This uses the verbose convention atLeastOneInstance<property> and allInstances<property>.
I'm open to alternatives, though.
|
Following up on an offhand remark from @rwy7, I do think that this can grow into computing edge-centric properties as well. I.e., this can get information about the instances and not just the modules/aggregate properties of all instances of modules. I'll leave this to future work as I don't want to overload this with too many things that don't have users. |
lib/Analysis/FIRRTLInstanceInfo.cpp
Outdated
| // determinend by their instantiations. | ||
| DenseSet<InstanceGraphNode *> visited; | ||
| for (auto *root : iGraph) { | ||
| for (auto *modIt : llvm::inverse_post_order_ext(root, visited)) { |
There was a problem hiding this comment.
I'm wondering, what's the difference between inverse and reverse post order ? ReversePostOrderTraversal seems to be used for topological sorted order, which is what the requirement is in this case.
There was a problem hiding this comment.
Presumably reverse of post order VS post-order on the inverse graph.
Good question! RPO is classic/ideal here.
Without cycles I'm not sure it matters (but would want to stare at it more...), and with them we can't guarantee convergence in a single walk (FWIW).
Using RPO, might be simplest conceptually as it's what we all expect here anyway and is more familiar for this sort of thing.
I was reaching for RPO in my suggestion, tbh.
FWIW we use inv post order elsewhere so hopefully thats correct/intentional for the same reason mentioned above, but might be good to check or ping those authors :).
There was a problem hiding this comment.
Empirically, I think it's the same in how it's being used here. Possibly, ReversePostOrder is slightly cleaner as it avoids having to do the pattern of nesting an inverse_post_order_ext under iteration over the nodes in the graph. I.e., the below can be replaced with what follows it:
DenseSet<InstanceGraphNode *> visited;
for (auto *root : iGraph) {
for (auto *modIt : llvm::inverse_post_order_ext(root, visited)) {
/* ... */
}
}ReversePostOrder<InstanceGraph *> rpo(iGraph);
for (auto *modIt : rpo) {
/* ... */
}I switched over to this in 8f1cedf.
There was a problem hiding this comment.
Chiming in a little late to this thread, but yes RPO and IPO should both visit the "parent" modules before the "children" modules, so it doesn't really matter which you use, but we usually use IPO for two reasons:
- RPO does a post-order walk, storing the elements to a SmallVector, and then walks it in reverse. IPO might be slightly faster because it doesn't need to build a vector of all modules.
- RPO will only visit modules which are reachable from the top level module (i.e. our instance graph trait implementation does not work well with multiple public modules), while the double loop used for IPO side steps this issue and visits all modules.
There was a problem hiding this comment.
Okay, thanks @youngar , this clarifies my confusion of RPO vs IPO, sticking with IPO makes sense.
There was a problem hiding this comment.
SGTM. Changed back in 3fbec15.
Add a new InstanceInfo analysis. This analysis provides common information which can be computed from a single walk of the InstanceGraph. The idea behind this is that there are a large amount of common information that different passes want to compute which all follows this same walk (and therefore has the same algorithmic complexity to compute it). Move all this information into a single, common analysis.
This analysis currently enables O(1) queries of if something is the DUT, is instantiated under the DUT, or is instantiated under a layer. More queries can be added as they are identified.
This new analysis naturally depends on the InstanceGraph analysis.