[Support] Maybe mark InstanceGraph GraphTraits deprecated
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
The `InstanceGraph` implements `llvm::GraphTraits`. That uses `getTopLevelNode()` as the root node of the graph, which does not contain all modules in the instance graph. Instead, the behaviour is dependent on concrete subclasses of the instance graph. The HW and FIRRTL dialects define this to be variants of "the top module" and "all public modules", which is usually not what we want in a pass. A pass is more likely to want to visit _all_ modules in the IR in post order (children before parents), not just a subset of the modules depending on whether things are transitively instantiated. The new `walkPostOrder` or `walkInversePostOrder` functions on `InstanceGraph` implement those walks.
It may be useful to mark at least `getTopLevelNode()` on the graph traits deprecated, with a comment about moving to the new walk functions. We shouldn't have any warnings in the code base after this. Only make this change if we have a valid alternative for all existing uses.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by auditing existing uses of InstanceGraph's llvm::GraphTraits and getTopLevelNode(), checking whether each has a valid replacement with walkPostOrder or walkInversePostOrder. Confirm that all uses can migrate without warnings; done means the deprecation is appropriate and the code base has no resulting warnings.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100