Skip to content

loader.cpp: support dumping IR/Graph visualizations from the commandline #148

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

Merged
merged 2 commits into from
Dec 14, 2017
Merged

Conversation

aweis
Copy link
Contributor

@aweis aweis commented Dec 14, 2017

This commit adds the following flags:

-dumpGraph - Prints Graph to stdout
-dumpGraphDAG=<file.dot> - Specify the file to export the Graph in DOT format
-dumpIR - Prints IR to stdout
-dumpIRDAG=<file.dot> - Specify the file to export the IR in DOT format

I had to slightly change the Module::dumpDAG() function to take in an
optional filepath. This is consistent with how the dumpDAG() function works
in Graph.cc.

This commit adds the following flags:

-dumpGraph                  - Prints Graph to stdout
-dumpGraphDAG=<file.dot>    - Specify the file to export the Graph in DOT format
-dumpIR                     - Prints IR to stdout
-dumpIRDAG=<file.dot>       - Specify the file to export the IR in DOT format

I had to slightly change the Module::dumpDAG() function to take in an
optional filepath. This is consistent with how the dumpDAG() function works
in Graph.cc.
@nadavrot
Copy link
Contributor

These changes are looking good! I like that you've made the interface for the graph and the IR the same. This is good. I have a small request. Can you please add an overload to the function that accepts no parameter instead of a default argument? The reason is that when you call the function in the debugger (p G_->dumpDAG()) it gives and error, because LLDB does not fill in default arguments. Overloading the function would allow us to call the function from the debugger.

Nadav suggested that we make our IR/DAG representations easier to debug
when using a debugger.  When you call the function in the debugger (p
G_->dumpDAG()) it would give an error before this change.
@aweis
Copy link
Contributor Author

aweis commented Dec 14, 2017

@nadavrot ^ I added the commit above with your suggestion. LMK if this is what you wanted and I will merge.

Copy link
Contributor

@nadavrot nadavrot left a comment

Choose a reason for hiding this comment

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

Nice!

@opti-mix
Copy link
Contributor

Looks nice!

My only concern is: these flags can be useful not only inside the loader, but as general purpose flags to be used with Glow-based tools, e.g. if you debug tests, etc. Thus having the definitions of these command-line options inside loader.cpp is may be not such a great idea. May be they should be in IR.cpp and Graph.cpp respectively? IMHO, the loader should just use these flags, but not define them.

@nadavrot
Copy link
Contributor

@opti-mix Roman, how will we know when to dump the DAG? In the loader we have a clear answer. In general code you never know if you want to dump before the auto-grad or after. Before optimizations or after. Before the target-specific-lowering or after, etc. I think that this is a good start.

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.

4 participants