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

[Relay][pass] call graph for relay #4922

Merged
merged 3 commits into from
Feb 26, 2020
Merged

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Feb 21, 2020

This PR introduces call graph for Relay. It is important for inter-procedural analysis, like inlining. It is largely inspired from the LLVM implementation here: https://llvm.org/doxygen/CallGraph_8h.html

@zhiics zhiics force-pushed the call_graph branch 2 times, most recently from 54af091 to 7e39f02 Compare February 22, 2020 02:07
@zhiics zhiics marked this pull request as ready for review February 22, 2020 05:14
@zhiics zhiics changed the title [WIP][Relay][pass] call graph for relay [Relay][pass] call graph for relay Feb 22, 2020
@zhiics
Copy link
Member Author

zhiics commented Feb 22, 2020

@icemelon
Copy link
Member

I wonder if we should create a folder for analysis and put call graph there instead of at the root folder of relay.

@zhiics
Copy link
Member Author

zhiics commented Feb 24, 2020

@icemelon9 Yeah, I had the same thoughts, but it seems we currently put other analysis passes under pass as well. We may need a refactoring for them.

@icemelon
Copy link
Member

I mean in python, there's analysis.py in the python/relay.

@zhiics
Copy link
Member Author

zhiics commented Feb 24, 2020

@icemelon9 ahh, make sense. We probably want to refactor the root directory to reflect the organization of c++ side. Currently, most of things are under the root directory.

@icemelon
Copy link
Member

Got it. We can do this in a follow up PR.

python/tvm/relay/call_graph.py Outdated Show resolved Hide resolved
src/relay/pass/call_graph.h Outdated Show resolved Hide resolved
python/tvm/relay/call_graph.py Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Feb 25, 2020

Note: it is fine it keep things as it is for now, we will likely need to move to the common ir after we finished the unified IR refactor, so call graph include both tir and relay

Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

Suggest to open an issue to track namespace refactoring after this is merged.

Copy link
Contributor

@anijain2305 anijain2305 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I have minor comments. I am just trying to understand the pass.

For my knowledge, how will this be used? You mentioned inlining. But, when do we have multiple functions in the same module?

src/relay/pass/call_graph.cc Outdated Show resolved Hide resolved
src/relay/pass/call_graph.cc Show resolved Hide resolved
src/relay/pass/call_graph.cc Outdated Show resolved Hide resolved
src/relay/pass/call_graph.cc Show resolved Hide resolved
src/relay/pass/call_graph.cc Outdated Show resolved Hide resolved
src/relay/pass/call_graph.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@anijain2305 anijain2305 left a comment

Choose a reason for hiding this comment

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

Requesting minor typo changes

@zhiics
Copy link
Member Author

zhiics commented Feb 25, 2020

@anijain2305 Thanks for reviewing.

Call graph is usually used for inter-procedural level analysis. For example, you might have multiple functions that form a call chain (a->b->c). In this case, we need to think about the orders of them when performing some optimizations. For inlining, we need to inline c to b first and then b->a.

We will have multiple functions in a module in many cases. One most common case is that whenever you use functions in prelude, you may have multiple functions in a module.

@anijain2305 anijain2305 self-assigned this Feb 25, 2020
@anijain2305 anijain2305 merged commit eba50ad into apache:master Feb 26, 2020
@anijain2305
Copy link
Contributor

Thanks @zhiics @icemelon9 @yzhliu This is merged

alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020
* call graph for relay

* CallGraphEntryNode->CallGraphEntry, __getitem__->print_var

* fix typos
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020
* call graph for relay

* CallGraphEntryNode->CallGraphEntry, __getitem__->print_var

* fix typos
zhiics added a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020
* call graph for relay

* CallGraphEntryNode->CallGraphEntry, __getitem__->print_var

* fix typos
@zhiics zhiics deleted the call_graph branch March 2, 2020 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants