[Relay] Add compiler pass tutorial docs #2746

weberlo · 2019-03-07T22:23:10Z

Here's a rough draft. There are some concerns I have already.

Is this too specific to the constant folding pass? I'd like to make it sufficiently general, but at the same time, depth on a single pass seems valuable.

Who should be the target audience? Do they already know how to use visitors and mutators? Or do they not know what an AST is?

Is constant folding a good example? @MarisaKirisame suggested the ANF-to-GNF pass might be a better fit.

Lemme know what you think and if I should ping anyone else.

@jroesch @slyubomirsky @MarisaKirisame

weberlo · 2019-03-07T22:23:50Z

docs/dev/relay_add_pass.rst

+
+TODO: The content of this section should be almost identical to the
+corresponding section in ``docs/dev/relay_add_op.rst``. Can we just link to
+there?


Also curious about this point.

Linking is good

slyubomirsky · 2019-03-07T22:30:55Z

docs/dev/relay_add_pass.rst

+
+Compiler passes can be used to collect data about Relay programs or transform
+them in various ways. The base class used to traverse programs is
+``ExprFunctor``. Depending on what you want to do, there are subclasses


I recommend using :code: for class names and other such constructs, as using the backticks without :code: results in italics rather than typewriter font.

slyubomirsky · 2019-03-07T22:33:07Z

docs/dev/relay_add_pass.rst

+
+Adding a Compiler Pass to Relay
+===============================
+


High-level remark about this introductory section: It should be emphasized that writing passes is the key to adding features to Relay because it is how you traverse the AST and inspect the program.

Another good point to emphasize is that Relay's "built in" core language features are implemented using the same pass interface, as a point to show how powerful those are: autodiff and type inference are implemented as passes.

slyubomirsky · 2019-03-07T22:34:23Z

docs/dev/relay_add_pass.rst

+
+Writing an Expression Visitor
+-----------------------------
+


It might be good to mention that the recommended practice for writing Visitors is to have a separate public interface method (like Check() in the example) rather than using VisitExpr(). This is because there sometimes needs to be some bookkeeping done before invoking the top-level recursion.

Also you should explain that VisitExpr() (no underscore) is the call necessary for recursive traversal, since the example does not include a recursive traversal (is there an easy enough case that does have one?)

slyubomirsky · 2019-03-07T22:36:07Z

docs/dev/relay_add_pass.rst

+In Relay, there are three types of nodes that we want to perform constant
+folding on:
+
+- ``LetNode``s


FYI I don't think RST permits having a character follow the backtick, so best to find a different way to phrase this (I don't know how to escape the "s").

slyubomirsky · 2019-03-07T22:36:54Z

docs/dev/relay_add_pass.rst

+- ``LetNode``s
+- ``TupleItemGetNode``s
+- ``CallNode``s
+


Code snippets would be more informative and illustrative for these paragraphs.

slyubomirsky · 2019-03-07T22:37:36Z

docs/dev/relay_add_pass.rst

+implementations for all node types. Each ``VisitExpr_`` method visits all of
+its node fields and creates a new node if any of the resulting fields have
+changed. Otherwise, the original node is returned.
+


I think the description of a mutator vs visitor should come earlier and be more detailed, since the visitor and mutator are very important base classes that anyone writing a pass will have to interact with.

slyubomirsky · 2019-03-07T22:38:52Z

docs/dev/relay_add_pass.rst

+corresponding section in ``docs/dev/relay_add_op.rst``. Can we just link to
+there?
+
+Summary


This summary seems unnecessary since it does not recap the points from the different sections (nor do I believe that should be at the end; a high-level summary would be best for the beginning).

slyubomirsky · 2019-03-07T22:39:23Z

src/relay/pass/fold_constant.cc

  }

 private:
  std::unordered_map<Expr, bool, NodeHash, NodeEqual> memo_;

+  void VisitExpr_(const ConstantNode* n) final {
+    memo_[GetRef<Constant>(n)] = true;


Is there a test case where the previous default behavior fails? This would be a good regression to add a test for.

I added this to the patch, mainly to discuss it.

I believe the code before is semantically equivalent to what I've changed it to, so I'm curious why the ConstantNode case was baked into Check. Is it just for performance purposes? If not, then I think this change just increases readability.

Oh, I see. It looks like it might be to avoid having constant nodes in the map (trying to reduce memory usage because constant nodes are a trivial case?). Not sure whether the change is necessary.

Yeah, I'm pretty sure it's an optimization now, so I don't think we should change the actual code. But in the tutorial, I'm thinking about having the code snippet differ from the actual code, just for for pedagogical reasons.

slyubomirsky · 2019-03-07T23:12:19Z

Re: your general questions in the top-level post, I think such a tutorial should try to be general in scope since Relay compiler passes are really powerful and we want users to make use of that power. I am also not sure precisely who comprises the target audience, but it would be safe to assume that even if they are familiar with the visitor pattern (as most programmers might be expected to be or would easily be able to read up on it), it would be helpful to give specific examples and explain how Relay's visitors are set up.

MarisaKirisame · 2019-03-08T18:01:14Z

LGTM

weberlo · 2019-03-10T21:14:07Z

Thanks for the feedback, @slyubomirsky. I've tried to incorporate it in the most recent commit, and I've added some more questions under TODOs.

Adding more reviewers! @srkreddy1238 @yongwww

Be as nitpicky as you can.

weberlo · 2019-03-20T21:27:45Z

@jroesch @slyubomirsky @MarisaKirisame @srkreddy1238 @yongwww

Pinging reviewers and adding @icemelon9

yongwww · 2019-03-21T16:21:16Z

adding @zhiics

MarisaKirisame · 2019-03-21T17:49:16Z

docs/dev/relay_add_pass.rst

+helper method ``ValueToExpr`` to allow us to place the evaluated expression
+back into the AST.
+
+TODO: Why does the ``CallNode`` case call ``VisitExpr_`` and not


ExprMutator::VisitExpr_(call) use the default implementation for call, which is just recurse
ExprMutator::VisitExpr(call) do dispatch and reach here again, which is infinite loop

zhiics · 2019-03-21T17:24:28Z

docs/dev/relay_add_pass.rst

+At a high level, there are three key components to writing a pass:
+
+- Creating one or more C++ classes that traverse the program
+- Registering an API endpoint with the ``TVM_REGISTER_API`` macro that performs the pass


We probably want to mention that it is a TVM packed function? Packed function has been mentioned in different docs.

zhiics · 2019-03-21T17:26:37Z

docs/dev/relay_add_pass.rst

+The base class used to traverse Relay programs is ``ExprFunctor``. The public
+interface it provides is a ``VisitExpr`` method that takes an expression and
+zero or more arguments and returns an instance of some type. When you extend
+this class, you define the AST traversal pattern by providing implementations


s/providing/overriding/

zhiics · 2019-03-21T17:29:10Z

docs/dev/relay_add_pass.rst

+~~~~~~~~~~~~~~~~~~~
+
+``ExprVisitor`` is for passes that don't modify the program and instead
+collect information about the program. With this class, ``VisitExpr`` and the


instead perform certain program analysis and collect information

zhiics · 2019-03-21T17:32:59Z

docs/dev/relay_add_pass.rst

+    }
+
+Note that we're calling ``VisitExpr`` and not ``VisitExpr_`` here. This way,
+routing to the appropriate implementation is taken care of for you, and you


I think we can probably mention here that calling VisitExpr will again dispatch to the correct or needed VisitExpr_ depending on the node type of each component of a IfNode. I meant we probably need to briefly talk about the functionality of VisitExpr` as well.

zhiics · 2019-03-21T17:39:38Z

docs/dev/relay_add_pass.rst

+      result_ = true;
+    }
+
+where ``result_`` is a field. In this case, we don't need to further recurse


I think this paragraph is not quite clear why we need result_ and I don't see how we stopped recursing on this visit.

zhiics · 2019-03-21T17:50:06Z

docs/dev/relay_add_pass.rst

+TODO: Why does the ``CallNode`` case call ``VisitExpr_`` and not
+``VisitExpr``?
+
+Registering an API Endpoint


Probably provide a pointer to the packed function doc?

MarisaKirisame · 2019-03-21T17:51:42Z

Add "add to pass.h" or "register in pass manager".

icemelon · 2019-03-29T23:39:31Z

@weberlo Could you update the pr based on the comments?

weberlo · 2019-03-30T06:51:26Z

docs/dev/relay_add_pass.rst

+
+And the pass can now be used in C++ and Python, though it's a good idea to
+wrap the API in Python, as described in :ref:`relay-add-op`. More detail
+about registration can be found in :doc:`runtime.md`.


One problem when building these docs is that the :doc:runtime.md reference doesn't get resolved. And we can't use the normal :ref:ayy-lmao mechanism to point to those docs, because they're in Markdown, and not RST. If nobody knows how RST<->Markdown interop works, I'll need to figure that out.

Can we just rewrite it into rst?

I've found it successful to add explicit references into docs and use :ref: to point to those references (see the Relay lang ref for examples)

Can we just rewrite it into rst?

Yeah. That seems like the way to go. Gonna rewrite it in ~~rust~~ rst today.

So uhh... why are the docs written in rst? It doesn't seem to highlight C or C++ very well and figuring out how to use its syntax feels like a puzzle.

weberlo · 2019-04-10T20:00:07Z

@jroesch @slyubomirsky @icemelon9 @zhiics Can y'all give another round of review? Let's try to get this merged in soon.

jroesch · 2019-04-15T22:59:21Z

I merged the initial version, improvements are welcome!

* Add Relay compiler pass tutorial docs * Add Python API hook wrapping step * Incorporate feedback * More doc iteration * Mooooore iteration * Rewrite `runtime.md` in rst

weberlo commented Mar 7, 2019

View reviewed changes

slyubomirsky reviewed Mar 7, 2019

View reviewed changes

MarisaKirisame reviewed Mar 21, 2019

View reviewed changes

zhiics reviewed Mar 21, 2019

View reviewed changes

icemelon added the status: need update need update based on feedbacks label Mar 29, 2019

weberlo commented Mar 30, 2019

View reviewed changes

weberlo added 6 commits April 11, 2019 15:15

Add Relay compiler pass tutorial docs

fbb2238

Add Python API hook wrapping step

fd7f54f

Incorporate feedback

72f5b10

More doc iteration

d2b3460

Mooooore iteration

29e7b0a

Rewrite runtime.md in rst

56ce9e9

weberlo force-pushed the add-docs branch from 41d5c10 to 56ce9e9 Compare April 11, 2019 22:16

jroesch approved these changes Apr 15, 2019

View reviewed changes

jroesch merged commit fbb7489 into apache:master Apr 15, 2019

tqchen mentioned this pull request Nov 8, 2019

[RELEASE][DRAFT] TVM v0.6 Release candidate #4259

Closed


		Adding a Compiler Pass to Relay
		===============================


		Writing an Expression Visitor
		-----------------------------

[Relay] Add compiler pass tutorial docs #2746

[Relay] Add compiler pass tutorial docs #2746

Conversation

weberlo commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

slyubomirsky Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky commented Mar 7, 2019

MarisaKirisame commented Mar 8, 2019

weberlo commented Mar 10, 2019

weberlo commented Mar 20, 2019

yongwww commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarisaKirisame commented Mar 21, 2019

icemelon commented Mar 29, 2019

weberlo Mar 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weberlo commented Apr 10, 2019

jroesch commented Apr 15, 2019

slyubomirsky Mar 7, 2019 •

edited

Loading

slyubomirsky Mar 7, 2019 •

edited

Loading

slyubomirsky Mar 7, 2019 •

edited

Loading

weberlo Mar 30, 2019 •

edited

Loading