Skip to content
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

Print out callgraph of a specific entry point #6354

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Mintyboi
Copy link
Contributor

@Mintyboi Mintyboi commented Feb 27, 2024

Allows us to generate / print out a callgraph for a specific entry point which we'd like to focus on.
Sample usage: wasm-opt foobar.wasm --print-call-graph -pa=callgraph-entry@foo > foo.dot

@Mintyboi Mintyboi changed the title Print out callgraph of a specific function Print out callgraph of a specific entry point Feb 27, 2024
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we don't have a test for this pass. If you want to extend it then I suggest we first land a PR that adds a test for the old functionality. Then this PR can land later, and the test will show nothing regressed.

}
}
Name caller = getPassOptions().getArgumentOrDefault(
"func",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a convention we use the pass in the name of the flag (unless the flag applies to more than one pass somehow). See for example the --jspi pass which uses jspi-* names:

auto stateChangingImports = String::trim(read_possible_response_file(
options.getArgumentOrDefault("jspi-imports", "")));
String::Split listedImports(stateChangingImports, ",");
// Find which exports should create a promise.
auto stateChangingExports = String::trim(read_possible_response_file(
options.getArgumentOrDefault("jspi-exports", "")));
String::Split listedExports(stateChangingExports, ",");
bool wasmSplit = options.hasArgument("jspi-split-module");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 10f8643


void constructCallgraph(Name caller) {
if (caller.toString().empty()) {
// whole callgraph, no filters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please expand in this comment, it is not obvious to me what "no filters" means.

Also please add a comment in the else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 10f8643

Module* module;
Function* currFunction;
std::set<Name> visitedTargets; // Used to avoid printing duplicate edges.
std::vector<Function*> allIndirectTargets;
CallPrinter(Module* module) : module(module) {
std::multimap<Name, Name> full_graph; // First param is the caller/parent, second is the callee/child
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::multimap<Name, Name> full_graph; // First param is the caller/parent, second is the callee/child
std::multimap<Name, Name> fullGraph; // First param is the caller/parent, second is the callee/child

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in 10f8643

@Mintyboi
Copy link
Contributor Author

It looks like we don't have a test for this pass. If you want to extend it then I suggest we first land a PR that adds a test for the old functionality. Then this PR can land later, and the test will show nothing regressed.

I found it useful for my use case and was sharing in the case where others will be able to benefit from it as well. If there's not much traction on this, I'll probably leave it as is.
But I do agree with adding a tests for old functionality on it! I'll see if I can work on that.

@kripken
Copy link
Member

kripken commented Feb 28, 2024

If there's not much traction on this, I'll probably leave it as is.

I do think this can be useful! My comments weren't meant to discourage you, but just to explain how I think it best to proceed to get this landed.

It's probably also fine to add a test in this PR for both cases, as the pass is simple enough.

}
Name caller = getPassOptions().getArgumentOrDefault(
"callgraph-entry",
"");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this flag in the top comment (line 20 or so).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants