Conversation
|
cc: @uenoku |
| // Note that post-order traversal on the instance graph never visit | ||
| // unreachable modules so it's safe to erase the module even though | ||
| // `modules` seems to be capturing module pointers. | ||
| module.erase(); |
There was a problem hiding this comment.
There is a certain situation where this erases modules still used by some symbol. I think until we have a proper analysis it's safer not to erase it (and we can run SymbolDCE anyway).
sv.verbatim "{{0}}" { symbols = [@baz] }
// Not reachable from the top level.
hw.module private @baz() {}There was a problem hiding this comment.
Does this also apply to the erase in eraseEmptyModule?
There was a problem hiding this comment.
Specifically do you mind if you could check an example in #10084
| liveElements.insert(newResult); | ||
|
|
||
| use->erase(); | ||
| instance->erase(); |
There was a problem hiding this comment.
could you update instance graph?
There was a problem hiding this comment.
LGTM other than module removal. For now I think we shouldn't remove empty modules as well; this has been a problem of FIRRTL IMDCE as well.
We need liveness analysis like https://github.com/llvm/llvm-project/blob/main/mlir/lib/Transforms/SymbolDCE.cpp). Basically if the module symbol is not referred by anything, it's ok to remove dead or empty modules.
|
Sorry for the delay. I changed so that the module is kept, but stripped. Or do you think that we should also keep the module signatures for non-executable modules? |
|
Thanks! I think it's not necessary to keep signatures (as inner symbol is required to refer to module ports). I think the current code looks good as is. |
| instanceGraph->erase(instanceGraphNode); | ||
| // module.erase(); | ||
| ++numErasedModules; |
2 participants
Add module rewriting functions and corresponding tests.
This also includes moving the helper
removeElementsAtIndicesto Utils from FIRRTL specific code.