llvm / llvm/circt

[Support] Maybe mark InstanceGraph GraphTraits deprecated

Open
#8,996 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.