Skip to content

[jit][ir] Add pretty printer for JIT IR#10319

Closed
driazati wants to merge 16 commits intopytorch:masterfrom
driazati:ir_pretty
Closed

[jit][ir] Add pretty printer for JIT IR#10319
driazati wants to merge 16 commits intopytorch:masterfrom
driazati:ir_pretty

Conversation

@driazati
Copy link
Contributor

@driazati driazati commented Aug 7, 2018

Adds some pretty-printing capability to the IR graph to make debugging easier/more human readable, see torch/csrc/jit/test_jit.cpp:925 and onwards for example outputs. Results aren't perfect yet but it's a start.

@driazati driazati changed the title Add pretty printer for JIT IR [ijt][ir] Add pretty printer for JIT IR Aug 7, 2018
@driazati driazati changed the title [ijt][ir] Add pretty printer for JIT IR [jit][ir] Add pretty printer for JIT IR Aug 7, 2018
goldsborough
goldsborough reviewed Aug 7, 2018
View reviewed changes
Copy link
Contributor

@goldsborough goldsborough left a comment

Choose a reason for hiding this comment

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

Some C++-level comments, leaving the semantic check to JIT folks

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Aug 7, 2018
View reviewed changes
torch/csrc/jit/test_jit.cpp Outdated

This comment was marked as off-topic.

Sign in to view

@zdevito
Copy link
Contributor

zdevito commented Aug 8, 2018

I don't have time for a detailed review right now, but a high-level comment: When storing meta-data for a particular pass like pretty printing, we do not modify structures in ir.h to hold it but instead create std::unordered_maps (e.g. std::unordred_map<Value*, std::string> original_name) that are local to the pass. This keeps implementation details of passes outside of the IR which is shared by many passes.

@ssnl ssnl added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 14, 2018
@ssnl
Copy link
Collaborator

ssnl commented Aug 14, 2018

@goldsborough @zdevito seems that the comments are addressed

ezyang
ezyang reviewed Aug 15, 2018
View reviewed changes
test/expect/TestScript.test_pretty_printer-while_if_test.expect Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Aug 15, 2018
View reviewed changes
test/expect/TestScript.test_pretty_printer-if_one.expect Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Aug 15, 2018
View reviewed changes
torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Aug 15, 2018
View reviewed changes
torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Aug 15, 2018
View reviewed changes
torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Aug 15, 2018
View reviewed changes
torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Aug 15, 2018
View reviewed changes
torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang requested changes Aug 15, 2018
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

I think there are some soundness issues.

@driazati
Copy link
Contributor Author

driazati commented Aug 21, 2018

Outputs are more verbose with the latest update but should be technically correct

zdevito
zdevito reviewed Aug 22, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. I have a bunch of minor comments to address to clean up the code. It would also be good to follow up with a PR that minimizes the number of assignment statements that need to be outputted in many circumstances by attempting to reuse variable names when possible.

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

suo
suo reviewed Aug 27, 2018
View reviewed changes
torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Aug 29, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@driazati
Copy link
Contributor Author

driazati commented Aug 31, 2018

@ezyang @zdevito Comments/correctness issues should be all addressed, is this ready to go?

zdevito
zdevito reviewed Sep 10, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

I think there are still a couple minor issues. Can you take a look and let me know?

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

apaszke
apaszke reviewed Sep 11, 2018
View reviewed changes
test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

driazati
driazati commented Sep 11, 2018
View reviewed changes
torch/csrc/jit/attributes.h Outdated

This comment was marked as off-topic.

Sign in to view

zdevito
zdevito reviewed Sep 12, 2018
View reviewed changes
torch/csrc/jit/passes/pretty_print.cpp Outdated

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Sep 12, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@driazati
Copy link
Contributor Author

driazati commented Sep 14, 2018

@pytorchbot retest this please

zdevito
zdevito approved these changes Sep 17, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

This looks good now!

facebook-github-bot
facebook-github-bot reviewed Sep 17, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

David Riazati added 15 commits September 17, 2018 14:15
Add pretty printer for JIT IR
b6df334 
Differential Revision: https://phabricator.intern.facebook.com/D9189808
Address comments / refactor
918689d 
Summary:
Fixed nits, moved tests out to `test_jit.py` and exposed functionality
to Python via `graph.pretty_print()`

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Address (most of) ezyang's comments
29bce3b 
Summary:
Changed intermediate names to prefix with "t" instead of "%"

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Refactor / fix correctness issues
7f119de 
Summary:
Output is more verbose / looks less nice but is now correct
Statements are generated surrounding loops to match loop dependencies to
outputs

Test Plan:
New test in test_jit.py, see expected outputs

Reviewers:

Subscribers:

Tasks:

Tags:
Address comments
1aebeea 
Summary:
* Moved all functionality to PrettyPrintPass struct, renamed methods
accordingly
* Removed unnecessary assignments of node outputs to loop outputs
* Removed alias for loop condition in case it is read after the loop

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Add Graph::dumpPretty()
4de0328 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Cache value names
0be4714 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Fix loop assignment bug
4d38022 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Update test output
df84310 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Addresss apaszke's comments
062f27b 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Move PrettyPrintPass to passes/pretty_print.h/cpp
feef151 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Alias block inputs to node outputs
ed66b83 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Cleanup
54f28fa 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Fix missing condition
47c2cd3 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Add bool casts to test
b021bb2 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
facebook-github-bot
facebook-github-bot reviewed Sep 17, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Fix rebase
18b69e2 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
facebook-github-bot
facebook-github-bot reviewed Sep 17, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ailzhang
Copy link
Contributor

ailzhang commented Sep 18, 2018

@pytorchbot retest this please

@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang requested changes

@suo suo suo left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zdevito zdevito zdevito approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

+2 more reviewers

@apaszke apaszke apaszke left review comments

@goldsborough goldsborough goldsborough left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@driazati @zdevito @ssnl @ailzhang @ezyang @suo @apaszke @facebook-github-bot @goldsborough