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.

Sign up for GitHub

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

Jump to bottom

[Relay][Text Format] Pretty Printer Smart Inlining #2881

Merged
merged 9 commits into from
Apr 13, 2019
Merged

[Relay][Text Format] Pretty Printer Smart Inlining #2881

merged 9 commits into from
Apr 13, 2019

Conversation

joshpoll
Copy link
Contributor

@joshpoll joshpoll commented Mar 22, 2019 •
edited
Loading

  • Adds smart inlining. Attempts to inline a node at the end of a scope or in a let value position when it is unique.
  • Removes GNF flag and debug printer. Smart inlining provides the benefits that turning GNF off provided, but without losing information about the original program.
  • Refactors dependency graph code out of to_a_normal_form.cc.

cc @vinx13 @wweic @MarisaKirisame @tqchen

@joshpoll joshpoll changed the title [Relay][Text Format] Pretty Printer Smart Inlining and Minor Improvements [Relay][Text Format] Pretty Printer Smart Inlining Mar 22, 2019
MarisaKirisame
MarisaKirisame reviewed Mar 25, 2019
View reviewed changes
src/relay/ir/pretty_printer.cc
@@ -181,9 +184,9 @@ class PrettyPrinter :

Doc PrintAttrs(const Attrs& attrs, const Expr& op);

Doc Print(const NodeRef& node, bool meta = false) {
Doc Print(const NodeRef& node, bool meta = false, bool try_inline = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make these two member of the language instead of passing them around.
If you pass them around you should change them. If they are constant save yourself the work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are flags for the Print function and are changed by the callers in specific instances. They are only changed for one invocation of Print at a time and importantly not for recursive invocations. I think they make the most sense as flags on Print rather than as instance variables.

wweic
wweic approved these changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@wweic wweic left a comment

Choose a reason for hiding this comment

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

lgtm.

@tqchen
Copy link
Member

tqchen commented Mar 31, 2019

@joshpoll Please clearly document the start inliner's behavior. Some suggestions:

  • Have a max depth which can be inlined
  • Have a white list of ops that can be inlined, set by op_attr, this way we don't mistakenly inline conv into another conv2d

@tqchen
Copy link
Member

tqchen commented Apr 9, 2019

@joshpoll please update this PR

@tqchen tqchen added the status: need update need update based on feedbacks label Apr 9, 2019
@joshpoll
Copy link
Contributor Author

@tqchen I have updated the PR documenting the inline heuristics. I don't think it's a good idea to add more until we have a better doc model that gives more layout options.

@tqchen tqchen merged commit dc97e52 into apache:master Apr 13, 2019
@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels Apr 13, 2019
@tqchen
Copy link
Member

tqchen commented Apr 13, 2019

Thanks, @joshpoll @MarisaKirisame @wweic this is now merged

wweic pushed a commit to wweic/tvm that referenced this pull request May 13, 2019
@joshpoll
[Relay][Text Format] Pretty Printer Smart Inlining (apache#2881)
7561043
wweic pushed a commit to neo-ai/tvm that referenced this pull request May 13, 2019
@joshpoll @wweic
[Relay][Text Format] Pretty Printer Smart Inlining (apache#2881)
742fc2e
@tqchen tqchen mentioned this pull request Nov 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wweic wweic wweic approved these changes

@tqchen tqchen tqchen approved these changes

@MarisaKirisame MarisaKirisame MarisaKirisame approved these changes

Assignees
No one assigned
Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshpoll @tqchen @wweic @MarisaKirisame