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][Text Format] Pretty Printer Smart Inlining #2881

Merged
merged 9 commits into from
Apr 13, 2019

Conversation

joshpoll
Copy link
Contributor

@joshpoll joshpoll commented Mar 22, 2019

  • 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
@@ -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.

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

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