[Relay] Start porting pass to the pass manager #3191

MarisaKirisame · 2019-05-14T21:04:14Z

@zhiics @jroesch is this fine? If so, I will convert the rest in the same manner.

zhiics · 2019-05-14T21:56:34Z

@MarisaKirisame Thank you for the effort. For registration this is okay to me. But there are still some problems we probably want to discuss:

I think that PassContext should be taken by every pass so that we can do things like error reporting in each pass func.
Should we invoke FuseOpPass(mod, ctx) and FuseOp(xx) separately, or with a unified interface? Separate invocation is okay to me.
FuseOp should be able to take Function as well. Should we treat it as a module pass?

@jroesch @tqchen Any other concerns?

MarisaKirisame · 2019-05-14T22:36:32Z

@zhiics I dont think it should be able to take function. Everything will work Module -> Module inside the pass manager, and if you want function to function, you can use the old api which will still be open.

zhiics · 2019-05-15T19:54:17Z

@MarisaKirisame Yes, you are right everything is Module->Module. But we also have FunctionPass which can take a pass function and apply it to each Function in a module, which is similar to what you are doing to update every Function here. It will return the new Module with updated functions.

MarisaKirisame · 2019-05-16T18:03:15Z

@zhiics I see.

MarisaKirisame · 2019-05-16T18:37:57Z

should functionpass return new Module instead of update inplace though? @zhiics @jroesch I am for the new Module approach.

zhiics · 2019-05-16T18:41:44Z

@MarisaKirisame I am okay with either way. I updated inplace because we usually cannot run multiple function level opts in parallel.

MarisaKirisame · 2019-05-16T18:47:45Z

@zhiics I see. I think leaving the old module intact is better because it allow one to debug by just looking at the intermediate Modules.

MarisaKirisame · 2019-05-16T19:04:14Z

@zhiics

Should we invoke FuseOpPass(mod, ctx) and FuseOp(xx) separately, or with a unified interface? Separate invocation is okay to me.
They should be unified into a FunctionPass. I will just do it in twostep: expose a FunctionPass api, which is relatively easy, and provide most of the benefit we want, and remove the old api, which require more work (and can be parallelized).

I think that PassContext should be taken by every pass so that we can do things like error reporting in each pass func.
You are correct. I will do it in the second PR.

FuseOp should be able to take Function as well. Should we treat it as a module pass?
It is fixed now. I just did not see that FunctionPass also work on Module earler.

zhiics · 2019-05-16T19:09:37Z

@MarisaKirisame I don't think we need to change the API to pass in a Module because it will execute on a Module

https://github.com/dmlc/tvm/blob/4b302cf88c9bfbdfbdd2015f46680678db2eae62/src/relay/pass/pass_manager.cc#L133

MarisaKirisame · 2019-05-16T19:28:30Z

@zhiics yes, but some pass might need to additionally take a module, to work on a function (to handle globalvar).
PartialEval is one of them. I think we should leave the possibility open.

zhiics · 2019-05-16T20:19:53Z

@MarisaKirisame Yes, I understand. As I mentioned in the RFC, it seems every pass will need Module as we want ErrorReporter.

EDIT: I was thinking if it is necessary to encode into to PassContext...

MarisaKirisame · 2019-05-16T21:21:22Z

@zhiics it is definitely possible, whetthrt we should or not is another issue. What was the design intent of PassContext?

zhiics · 2019-05-16T21:37:05Z

@MarisaKirisame Let's bring @jroesch in discussion. PassContext was designed to only take some internal information that is used by a pass. Originally we proposed to have PassState instead which took Module as well.

tqchen · 2019-05-22T17:30:27Z

include/tvm/relay/pass.h

 /*! \brief Aggressive constant propagation/constant folding/inlining.
 * It will do as much computation in compile time as possible.
 * It has two benefit: remove runtime overhead, and allow more optimization (typically fusion).
 * As a side effect, code size will explode.
 */
 Expr PartialEval(const Expr& e);
+
+inline pass::Pass PartialEvalPass() {


do not use inline function and put all implementation in the corresponding pass file.

MarisaKirisame · 2019-05-23T21:13:47Z

@tqchen I address your comments. can you give another round of review?

zhiics · 2019-05-23T21:29:31Z

@MarisaKirisame This looks good to me now. Do we have more passes to register?

MarisaKirisame · 2019-05-23T21:33:57Z

@zhiics yes. for example, simplify inference is not exposed in the C++ side.
there is a few more, and I dont think chasing through them is a good idea as they scatter accross the code base.
I think I can do them as I move the old optimize function into the new style (in the next PR), is it fine with you?

zhiics · 2019-05-23T21:39:21Z

@MarisaKirisame Yes, that's totally fine to me. Thank you. Another thing we need to keep in mind or probably add to the RFC is that we should add PassContext to the passes so that they can take internal thing like error reporting.

tqchen · 2019-05-24T01:22:31Z

include/tvm/relay/pass.h

@@ -271,13 +272,17 @@ TVM_DLL tvm::Array<TypeVar> AllTypeVars(const Type& t, const Module& mod);
 */
 TVM_DLL Expr DeadCodeElimination(const Expr& e);

+transform::Pass DeadCodeEliminationPass();


given that transform namespace has been merged in. Please move them to transform.h . Under the same name

Pass DeadCodeElimination();

tqchen · 2019-05-24T01:23:38Z

src/relay/pass/pass.cc

+using namespace runtime;
+using std::function;
+
+Pass DeadCodeEliminationPass() {


Instead of putting these in pass.cc , directly move it to dead_code.cc Note that we might remove the original DeadCodeElimination function and only keep this one

tqchen · 2019-05-24T06:00:35Z

include/tvm/relay/transform.h

    int opt_level,
    const std::string& name,
    const tvm::Array<tvm::Expr>& required);

+Pass DeadCodeElimination();


document each function using the doxygen format

tqchen · 2019-05-24T06:02:37Z

include/tvm/relay/pass.h

+   *
+   * Implements structural hashing of a Relay type.
+   *
+   *  \param type the type to hash.


seems the location of StructualHash does not have to change

I wanna move the big one down, and have the small one together, so it is easier to read.

tqchen · 2019-05-24T06:03:00Z

include/tvm/relay/transform.h

    int opt_level,
    const std::string& name,
    const tvm::Array<tvm::Expr>& required);

+Pass DeadCodeElimination();


Mark the declaration with prefix TVM_DLL so that it is exposed in the dynamic library.

tqchen · 2019-05-24T19:16:50Z

include/tvm/relay/transform.h

+ *
+ * \return The pass.
+ */
+TVM_DLL Pass FuseOps(int fuse_opt_level);


Let us allow fuse_opt_level=-1 (automatic mode) which allows the op_level to equal PassContext::Current()->opt_level

update save do fix build lint do do fix lint save address comment ci save fix conflict

tqchen · 2019-05-24T23:43:31Z

Thanks, @MarisaKirisame , this is merged

zhiics mentioned this pull request May 22, 2019

[relay][pass manager] Open transform namespace #3226

Merged

tqchen requested changes May 22, 2019

View reviewed changes

MarisaKirisame force-pushed the port branch from ae86024 to b7cd5a9 Compare May 23, 2019 20:30

tqchen requested changes May 24, 2019

View reviewed changes

tqchen reviewed May 24, 2019

View reviewed changes

save

bbdfb36

update save do fix build lint do do fix lint save address comment ci save fix conflict

MarisaKirisame force-pushed the port branch from ab4a40a to bbdfb36 Compare May 24, 2019 20:27

tqchen approved these changes May 24, 2019

View reviewed changes

update comment

3f1560f

tqchen merged commit 89a88c5 into apache:master May 24, 2019

tqchen added the status: accepted label May 24, 2019

wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019

[Relay] Start porting pass to the pass manager (apache#3191)

f8c4cb9

wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019

[Relay] Start porting pass to the pass manager (apache#3191)

c9f11e8

MarisaKirisame deleted the port branch July 13, 2019 20:59

tqchen mentioned this pull request Nov 8, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay] Start porting pass to the pass manager #3191

[Relay] Start porting pass to the pass manager #3191

MarisaKirisame commented May 14, 2019

zhiics commented May 14, 2019 •

edited

Loading

MarisaKirisame commented May 14, 2019

zhiics commented May 15, 2019 •

edited

Loading

MarisaKirisame commented May 16, 2019 •

edited

Loading

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019

MarisaKirisame commented May 16, 2019

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019 •

edited

Loading

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019

tqchen May 22, 2019

MarisaKirisame commented May 23, 2019 •

edited

Loading

zhiics commented May 23, 2019

MarisaKirisame commented May 23, 2019

zhiics commented May 23, 2019 •

edited

Loading

tqchen May 24, 2019

tqchen May 24, 2019

tqchen May 24, 2019

tqchen May 24, 2019

MarisaKirisame May 24, 2019

tqchen May 24, 2019

tqchen May 24, 2019

tqchen commented May 24, 2019

[Relay] Start porting pass to the pass manager #3191

[Relay] Start porting pass to the pass manager #3191

Conversation

MarisaKirisame commented May 14, 2019

zhiics commented May 14, 2019 • edited Loading

MarisaKirisame commented May 14, 2019

zhiics commented May 15, 2019 • edited Loading

MarisaKirisame commented May 16, 2019 • edited Loading

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019

MarisaKirisame commented May 16, 2019

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019 • edited Loading

MarisaKirisame commented May 16, 2019

zhiics commented May 16, 2019

Choose a reason for hiding this comment

MarisaKirisame commented May 23, 2019 • edited Loading

zhiics commented May 23, 2019

MarisaKirisame commented May 23, 2019

zhiics commented May 23, 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

tqchen commented May 24, 2019

zhiics commented May 14, 2019 •

edited

Loading

zhiics commented May 15, 2019 •

edited

Loading

MarisaKirisame commented May 16, 2019 •

edited

Loading

zhiics commented May 16, 2019 •

edited

Loading

MarisaKirisame commented May 23, 2019 •

edited

Loading

zhiics commented May 23, 2019 •

edited

Loading