-
Notifications
You must be signed in to change notification settings - Fork 859
Use global effects call graph logic for --print-call-graph #8689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
stevenfontanella
wants to merge
6
commits into
main
Choose a base branch
from
call-graph-pass
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+172
−3,108
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
3fc109c
Add pass to generate call graph
stevenfontanella 42f8b46
Start replacing existing pass
stevenfontanella 8e0385e
Replace existing pass
stevenfontanella e1d1b56
Add some tests and rename
stevenfontanella 7de0de7
Rename
stevenfontanella f63d1e7
Fix
stevenfontanella File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| ;; RUN: foreach %s %t wasm-opt -all --print-call-graph | filecheck %s | ||
| ;; RUN: foreach %s %t wasm-opt -all --print-call-graph --closed-world | filecheck %s --check-prefix CLOSED | ||
|
|
||
| (module | ||
| (type $indirect-type (func (param i32))) | ||
|
|
||
| (table 1 1 funcref) | ||
| (elem $indirect-1 (i32.const 0)) | ||
|
|
||
| (func $indirect-1 (type $indirect-type) (param i32) | ||
| ) | ||
|
|
||
| (func $indirect-2 (type $indirect-type) (param i32) | ||
| ) | ||
|
|
||
| (func $A (param $ref (ref $indirect-type)) | ||
| (call_ref $indirect-type (i32.const 42) (local.get $ref)) | ||
| (call $B (local.get $ref)) | ||
| ) | ||
|
|
||
| (func $B (param $ref (ref $indirect-type)) | ||
| (call_indirect (type $indirect-type) (i32.const 0) (i32.const 0)) | ||
| ) | ||
| ) | ||
|
|
||
| ;; CHECK: digraph CallGraph { | ||
| ;; CHECK-NEXT: "A" [kind="function"]; | ||
| ;; CHECK-NEXT: "B" [kind="function"]; | ||
| ;; CHECK-NEXT: "indirect-1" [kind="function"]; | ||
| ;; CHECK-NEXT: "indirect-2" [kind="function"]; | ||
| ;; CHECK-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"]; | ||
| ;; CHECK-NEXT: } | ||
|
|
||
| ;; CLOSED: digraph CallGraph { | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param (ref $func.1))))" [kind="type"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" [kind="type"]; | ||
| ;; CLOSED-NEXT: "A" [kind="function"]; | ||
| ;; CLOSED-NEXT: "B" [kind="function"]; | ||
| ;; CLOSED-NEXT: "indirect-1" [kind="function"]; | ||
| ;; CLOSED-NEXT: "indirect-2" [kind="function"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param (ref $func.1))))" -> "A" [style="dashed", color="green", kind="implementation"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param (ref $func.1))))" -> "B" [style="dashed", color="green", kind="implementation"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" -> "indirect-1" [style="dashed", color="green", kind="implementation"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" -> "indirect-2" [style="dashed", color="green", kind="implementation"]; | ||
| ;; CLOSED-NEXT: "A" -> "(type $func.0 (func (param i32)))" [style="dotted", color="black", kind="indirect"]; | ||
| ;; CLOSED-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"]; | ||
| ;; CLOSED-NEXT: "B" -> "(type $func.0 (func (param i32)))" [style="dotted", color="black", kind="indirect"]; | ||
| ;; CLOSED-NEXT: } | ||
|
|
||
| ;; Test cycles | ||
| (module | ||
| (type $indirect-type (func (param i32))) | ||
| (table 1 1 funcref) | ||
|
|
||
| (func $A | ||
| (call $B) | ||
| ) | ||
| (func $B | ||
| (call $A) | ||
| ) | ||
|
|
||
| (func $indirect-cycle (type $indirect-type) (param $x i32) | ||
| (call_indirect (type $indirect-type) (local.get $x) (i32.const 0)) | ||
| ) | ||
| (elem $indirect-cycle (i32.const 0)) | ||
| ) | ||
|
|
||
| ;; CHECK: digraph CallGraph { | ||
| ;; CHECK-NEXT: "A" [kind="function"]; | ||
| ;; CHECK-NEXT: "B" [kind="function"]; | ||
| ;; CHECK-NEXT: "indirect-cycle" [kind="function"]; | ||
| ;; CHECK-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"]; | ||
| ;; CHECK-NEXT: "B" -> "A" [style="solid", color="black", kind="direct"]; | ||
| ;; CHECK-NEXT: } | ||
|
|
||
| ;; CLOSED: digraph CallGraph { | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" [kind="type"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func))" [kind="type"]; | ||
| ;; CLOSED-NEXT: "A" [kind="function"]; | ||
| ;; CLOSED-NEXT: "B" [kind="function"]; | ||
| ;; CLOSED-NEXT: "indirect-cycle" [kind="function"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" -> "indirect-cycle" [style="dashed", color="green", kind="implementation"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func))" -> "A" [style="dashed", color="green", kind="implementation"]; | ||
| ;; CLOSED-NEXT: "(type $func.0 (func))" -> "B" [style="dashed", color="green", kind="implementation"]; | ||
| ;; CLOSED-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"]; | ||
| ;; CLOSED-NEXT: "B" -> "A" [style="solid", color="black", kind="direct"]; | ||
| ;; CLOSED-NEXT: "indirect-cycle" -> "(type $func.0 (func (param i32)))" [style="dotted", color="black", kind="indirect"]; | ||
| ;; CLOSED-NEXT: } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait why is PrintCallGraph now inside GlobalEffects..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mention this in the PR description, I chose to keep it here since the logic is somewhat specific to GlobalEffects. e.g. it's not obvious in general that a call graph would have edges from HeapType -> Function to indicate that the function has a given type. I could extract out the call graph logic into a shared header and then use that in both GlobalEffects and a call graph printing pass, but it seems a bit weird since I'm not sure if the call graph generation logic would be applicable anywhere else. I'm ok with going either way though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Well, my concern is that it seems very surprising to see a general Print* method inside GlobalEffects. PrintGlobalEffects might make sense, but this is a lot more general?
What would go into the shared header? It seems like the existing PrintCallGraph code can stay where it is, and just add edges from heap types to functions - is that more than a few lines of code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my purposes, I'd like to look at the exact same call graph that GlobalEffects is already generating, so I think I'd move this code from GlobalEffects (plus the filtering logic from #8644 once it's in) into a header and then re-use that between GlobalEffects and the print call graph pass. Does it make sense to extract it out or should I keep it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the current PrintCallGraph pass already scans calls, and I don't think it needs effects - what would it use from the code that you linked to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the highlighted lines were a bit imprecise. I'd want to extract out
buildCallGraph, but that requires us to track the indirect + direct calls from each function which we currently do inanalyzeFuncs(we pass funcInfos to generateCallGraph). The current GlobalEffects also computes effects for each function at the same time that it notes the indirect + direct calls. If we extracted it out, I guess we'd just do a second pass over the functions to do that? Or maybe I'd introduce some way of composing ParallelFunctionAnalysis and running them together in one pass.Basically if I extract out
buildCallGraphit seems like we'd have to compute the call graph and the effects in two separate passes over the module's functions, is that ok?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, now I see, so it would track the indirect calls too.
Yes, extracting something to do that makes sense.
In fact, how about extending the existing
CallGraphPropertyAnalysisfrommodule-utils.h, with indirect call tracking? Then all existing users would benefit automatically from a more precise analysis (at least in closed world). And then both PrintCallGraph and GlobalEffects can use that utility.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, that looks reasonable. Let me see if I can add it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @kripken that pulling this logic out into a general location sounds fine. Even if HeapType -> Function edges are not necessarily the most intuitive things, I think they are a sufficiently general and expressive way to represent this information that we may as well adopt them everywhere we care about call graphs.