feat: Add CallGraph struct, and dead-function-removal pass #1796

acl-cqc · 2024-12-17T12:54:12Z

Closes #1753.

Tested by replacing use of remove_polyfuncs in feat!: Add monomorphization pass #1733; remove_polyfuncs is kept but deprecated and untested.
Note the CallGraph struct has no public accessors/etc. yet. I propose to leave this until we have more uses, as I found it hard to expose suitable for using petgraph utilities such as Bfs.

The longer-term plan is to integrate with #1672 by doing dataflow analysis and transforming to remove Calls that are locally/dynamically impossible, which will make more functions statically-unreachable and hence removable by this.

…docs

codecov · 2024-12-17T12:57:00Z

Codecov Report

Attention: Patch coverage is 93.68421% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.60%. Comparing base (1f841ae) to head (f8008d9).

Files with missing lines	Patch %	Lines
hugr-passes/src/call_graph.rs	93.40%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1796      +/-   ##
==========================================
- Coverage   86.63%   86.60%   -0.04%     
==========================================
  Files         191      192       +1     
  Lines       34738    34830      +92     
  Branches    31571    31663      +92     
==========================================
+ Hits        30097    30166      +69     
- Misses       2940     2961      +21     
- Partials     1701     1703       +2

Flag	Coverage Δ
python	`92.42% <ø> (ø)`
rust	`86.02% <93.68%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ss2165 · 2024-12-17T13:59:26Z

can we deprecate remove_polyfuncs instead so we can include this in a non-breaking release?

…and suppress warning on re-export

acl-cqc · 2024-12-17T17:41:34Z

can we deprecate remove_polyfuncs instead so we can include this in a non-breaking release?

Yes, well, we can, and I have, but I admit I am not singing the praises of the tooling here - deprecate has a "since" tag for tools that might use it, but IIUC I have to guess what the next version number will be (i.e. the version in which it'll be deprecated); and the re-export causes problems (I have to suppress a warning there - I tried putting the #[deprecated] only on the re-export but that did not generate a warning even when I used the function via the re-export). If I'm doing this wrong @ss2165 then please shout...

tatiana-s

Generally looks good to me, just one comment mainly for my understanding.

tatiana-s · 2024-12-18T15:04:58Z

hugr-passes/src/call_graph.rs

+        fn traverse(
+            h: &impl HugrView,
+            node: Node,
+            enclosing: NodeIndex<u32>,


Just for clarification - is enclosing the node in the call graph corresponding to the node node in the hugr or something else?

It's the (callgraph representation of) the nearest enclosing FuncDefn, so maybe the node's parent or an ancestor. I'll rename...

…vel::default

doug-q · 2024-12-19T09:58:02Z

hugr-passes/src/call_graph.rs

+            roots.extend(h.children(h.root()).filter(|n| {
+                h.get_optype(*n)
+                    .as_func_defn()
+                    .is_some_and(|fd| fd.name == "main")
+            }));
+            assert_eq!(roots.len(), 1, "No entry_points for Module and no `main`");


I think this should be the responsibility of the caller.

I think panicking here is a bit much, and that it would be better to just delete everything.

This means we panic on an empty module (e.g. Hugr::default() is an empty module). One can imagine that linker might invoke this pass at some point, when deleting everything would be fine.

doug-q · 2024-12-19T10:00:36Z

hugr-passes/src/call_graph.rs

+/// * If the Hugr is non-Module-rooted and `entry_points` is non-empty
+/// * If the Hugr is Module-rooted, but does not declare `main`, and `entry_points` is empty
+/// * If the Hugr is Module-rooted, and `entry_points` is non-empty but contains nodes that
+///      are not [FuncDefn](OpType::FuncDefn)s


As above, I don't think the second should panic.

I think the interface would be cleaner if entry_points, maybe with a different name: must be FuncDefn or FuncDecl nodes that are immediate children of the root.

Now the first panic goes away, and the third would be an error with the offending nodes.

doug-q · 2024-12-19T10:03:21Z

hugr-passes/src/call_graph.rs

+    }
+
+    /// Runs the pass (see [remove_dead_funcs]) with this configuration
+    pub fn run<H: HugrMut>(&self, hugr: &mut H) -> Result<(), ValidatePassError> {


I think it would be better to declare a new error type with [non_exhaustive] so we have the freedom to add variants later without breaking the interface.

doug-q · 2024-12-19T10:07:16Z

hugr-passes/src/call_graph.rs

+            assert_eq!(roots.len(), 1, "No entry_points for Module and no `main`");
+        }
+        let mut roots = roots.into_iter().map(|i| cg.node_to_g.get(&i).unwrap());
+        let mut b = Bfs::new(&cg.g, *roots.next().unwrap());


Why Bfs? I think Dfs would be marginally better because more memory efficient

doug-q · 2024-12-19T10:08:28Z

hugr-passes/src/call_graph.rs

+
+    /// Adds new entry points for a Module-rooted Hugr,
+    /// i.e., FuncDefns that are children of the root.
+    pub fn with_module_entry_points(


If you accept my suggestions on the entry_points interface this should get a new name

doug-q · 2024-12-19T10:22:08Z

hugr-passes/src/call_graph.rs

+
+#[derive(Debug, Clone, Default)]
+/// A configuration for the Dead Function Removal pass.
+pub struct RemoveDeadFuncsPass {


I would prefer for this to live in a new module. CallGraph is more general than this.

doug-q · 2024-12-19T10:24:15Z

hugr-passes/src/call_graph.rs

+    let reachable = reachable_funcs(&CallGraph::new(h), h, entry_points).collect::<HashSet<_>>();
+    let unreachable = h
+        .nodes()
+        .filter(|n| h.get_optype(*n).is_func_defn() && !reachable.contains(n))


We should remove dead FuncDecls too

doug-q · 2024-12-19T10:28:24Z

hugr-passes/src/call_graph.rs

+///      are not [FuncDefn](OpType::FuncDefn)s
+pub fn remove_dead_funcs(h: &mut impl HugrMut, entry_points: impl IntoIterator<Item = Node>) {
+    let reachable = reachable_funcs(&CallGraph::new(h), h, entry_points).collect::<HashSet<_>>();
+    let unreachable = h


I think this definition of unreachable includes the root of a FuncDefn-rooted hugr? Surely worth a test case

doug-q · 2024-12-19T10:29:39Z

hugr-passes/src/call_graph.rs

+    h: &impl HugrView,
+    entry_points: impl IntoIterator<Item = Node>,
+) -> impl Iterator<Item = Node> + 'a {
+    let mut roots = entry_points.into_iter().collect_vec();


It's a little thing, but if we didn't have to materialize this vec then the whole algorithm would stream. I think that would involve having to silently ignore any non-func entry_points, or at least you'd have to work hard to track them. probably not worth it.

doug-q · 2024-12-19T10:31:55Z

hugr-passes/src/call_graph.rs

+/// A configuration for the Dead Function Removal pass.
+pub struct RemoveDeadFuncsPass {
+    validation: ValidationLevel,
+    entry_points: Vec<Node>,


Did you consider making this a HashSet? it would make with_module_entry_points truly idempotent.

doug-q · 2024-12-19T10:37:03Z

hugr-passes/src/call_graph.rs

+/// [FuncDefn]: OpType::FuncDefn
+/// [LoadFunction]: OpType::LoadFunction
+pub struct CallGraph {
+    g: Graph<Node, CallGraphEdge>,


let's add a getter for this

doug-q · 2024-12-19T10:39:00Z

Did you consider tracking in the Node weight whether it's a FuncDecl or a FuncDefn?

acl-cqc added 4 commits December 17, 2024 11:57

Add call_graph.rs, start writing docs

3a0ee56

roots == Some(empty) meaningless => make non-Opt; pub CallGraphEdge; …

fcd5321

…docs

Remove remove_polyfuncs

c497e4d

Warn on missing docs

c3dd939

acl-cqc changed the title ~~feat: [Slightly WIP] Add CallGraph struct, and dead-function-removal pass~~ feat!: [Slightly WIP] Add CallGraph struct, and dead-function-removal pass Dec 17, 2024

Reinstate remove_polyfuncs but deprecate: guess next version number, …

3bc33bc

…and suppress warning on re-export

acl-cqc changed the title ~~feat!: [Slightly WIP] Add CallGraph struct, and dead-function-removal pass~~ feat: [Slightly WIP] Add CallGraph struct, and dead-function-removal pass Dec 17, 2024

Test module entry_points

1e95bc6

acl-cqc changed the title ~~feat: [Slightly WIP] Add CallGraph struct, and dead-function-removal pass~~ feat: Add CallGraph struct, and dead-function-removal pass Dec 17, 2024

Move reachable_funcs outside of CallGraph

9061dc9

acl-cqc force-pushed the acl/remove_dead_funcs branch from 31a7b44 to 08d9ebc Compare December 17, 2024 18:58

Rename entry_points<->roots, use extend + assert

e29ffa2

acl-cqc force-pushed the acl/remove_dead_funcs branch from 08d9ebc to e29ffa2 Compare December 17, 2024 19:02

Merge branch 'main' into acl/remove_dead_funcs

5f89cac

acl-cqc marked this pull request as ready for review December 17, 2024 19:11

acl-cqc requested a review from a team as a code owner December 17, 2024 19:11

acl-cqc requested a review from tatiana-s December 17, 2024 19:11

tatiana-s reviewed Dec 18, 2024

View reviewed changes

tatiana-s requested a review from doug-q December 18, 2024 15:39

acl-cqc added 4 commits December 18, 2024 15:47

Merge 'origin/main' into acl/remove_dead_funcs, deprecation msgs

4ee87aa

Add RemoveDeadFuncsPass. TODO make remove_dead_funcs use ValidationLe…

220bf67

…vel::default

enclosing{=>_func}, switch order, comment

466123d

Use Pass in tests

f8008d9

doug-q reviewed Dec 19, 2024

View reviewed changes

doug-q closed this Dec 19, 2024

doug-q reopened this Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add CallGraph struct, and dead-function-removal pass #1796

feat: Add CallGraph struct, and dead-function-removal pass #1796

acl-cqc commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

ss2165 commented Dec 17, 2024

acl-cqc commented Dec 17, 2024

tatiana-s left a comment

tatiana-s Dec 18, 2024

acl-cqc Dec 18, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024 •

edited

Loading

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q Dec 19, 2024

doug-q commented Dec 19, 2024

feat: Add CallGraph struct, and dead-function-removal pass #1796

Are you sure you want to change the base?

feat: Add CallGraph struct, and dead-function-removal pass #1796

Conversation

acl-cqc commented Dec 17, 2024 • edited Loading

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

ss2165 commented Dec 17, 2024

acl-cqc commented Dec 17, 2024

tatiana-s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q commented Dec 19, 2024

acl-cqc commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

doug-q Dec 19, 2024 •

edited

Loading