[Relay] Partial Eval now Support Interprocedural optimization and has termination check. #3033

MarisaKirisame · 2019-04-16T18:46:05Z

@nhynes @wweic @icemelon9 @junrushao1994 @ZihengJiang @jroesch @tqchen can you guys review?

include/tvm/relay/expr.h

MarisaKirisame · 2019-05-08T04:38:06Z

@tqchen can you give another round of review?

tqchen · 2019-05-16T03:24:49Z

@MarisaKirisame please rebase. @wweic @jroesch please help review this PR

wweic

some general cents. will do more detailed review in the next few days.

wweic · 2019-05-17T06:22:42Z

src/relay/ir/expr.cc

@@ -220,8 +220,8 @@ TVM_REGISTER_API("relay._make.Call")

 TVM_STATIC_IR_FUNCTOR_REGISTER(IRPrinter, vtable)
 .set_dispatch<CallNode>([](const CallNode* node, tvm::IRPrinter* p) {
-  p->stream << "CallNode(" << node->op << ", " << node->args << ", "
-    << node->attrs << ", " << node->type_args << ")";
+    p->stream << "CallNode(" << node->op << ", " << node->args << ", "


Better use the command in https://github.com/dmlc/tvm/blob/master/.clang-format to do the formatting so we maintain a consistent code style. The indentation style is 2 space I think.

It does not work - the result is completely different from all other files.

Probably due to we use different rules for macro.

src/relay/pass/partial_eval.cc

tests/python/relay/test_pass_partial_eval.py

wweic · 2019-05-17T17:28:04Z

src/relay/pass/partial_eval.cc

 private:
  Environment env_;
+  Module mod_;
+  std::unordered_map<GlobalVar, PStatic, NodeHash, NodeEqual> gv_map_;
+  /*! Termination checking is done as follow:


Is there a test that verifies termination checking(no infinite loop)?

there is test_map and test_loop. However, none of the test is working right now as the vm merge break this. I will fix them.

They are fixed.

MarisaKirisame · 2019-05-17T17:52:43Z

note to self: dead code elimination dont do inline, so we need an inline pass to make test work again.

MarisaKirisame · 2019-05-28T01:17:58Z

@nhynes @icemelon9 can you guys help review?

MarisaKirisame · 2019-05-30T17:20:39Z

@tqchen @jroesch @slyubomirsky can you guys give it a round of review?

slyubomirsky · 2019-05-31T01:31:12Z

src/relay/pass/partial_eval.cc

@@ -383,15 +388,78 @@ FInterpreter CPUInterpreter() {
  return CreateInterpreter(Module(nullptr), CPUContext(), target);
 }

+bool IsAtomic(const Expr& e) {
+  return e.as<VarNode>() || e.as<OpNode>() || e.as<ConstructorNode>() || e.as<GlobalVarNode>();


Are constant nodes atomic?

They arent: they are big.

slyubomirsky · 2019-05-31T01:33:44Z

I think it would be good to include some explanation as to the use of the creation time mechanism and why it's important for termination checking, as that was all very difficult for me to understand. More varied test cases (given how complicated the feature is) would be good too.

MarisaKirisame · 2019-06-05T04:32:17Z

@slyubomirsky it is not complicated at all! the high level idea is that the argument must be strictly decreasing in some order to be terminating. We choose the order to simply be the order it is created. This work for lots of the case, because for any adt, the outer node is newer then the inner node. So, as long as any of the argument is decreasing, it is good. To include the case where argument a decrease, argument b increase, then argument b increase, argument a decrease, we specify that decrease mean 'globally decrease' (etc: smaller then all seen time at that argument).
I will look at the comment and update it.

MarisaKirisame · 2019-06-05T04:40:55Z

@slyubomirsky I revisited the comment and couldnt figure out how to change it. Anything that is particularly confusing?

slyubomirsky · 2019-06-05T05:00:21Z

I didn't mean that anything is confusing in itself, just that there should somewhere be a note explaining what the creation times are for and what it does. It took me a fair bit of reading and rereading and thinking to figure out how it worked and that is probably not good for future maintainability.

wweic · 2019-06-06T08:17:07Z

@MarisaKirisame How do we prevent infinite loop of PE? One case I can think of is a function with 2 arguments, the series of created_time that can cause infinite loop is (1, 1), (0, 2),(2, 1), (1, 3), (3, 2), (2, 4), .... 1st argument grow its created time by 1 at odd position, 2nd argument grow its created time by 1 at even position, so it can trick the PE to recurse all the time.
1st argument: 1, 0, 2, 1, 3, 2, 4, 3, 5, 4
2nd argument: 1, 2, 1, 3, 2, 4, 3, 5, 4, 6

Do you think is this case possible and how do we prevent that?

MarisaKirisame · 2019-06-07T17:59:39Z

@wweic

the series of created_time that can cause infinite loop is (1, 1), (0, 2),(2, 1), (1, 3), (3, 2), (2, 4), ...

the lowest_bound is (1, 1), (0, 1), then (2, 1) is checked and found that it does not decrease any argument. I specifically design it to avoid this. I had no idea how to fix the comment though. Can you make some suggestion?

MarisaKirisame · 2019-06-07T20:06:05Z

@wweic I had add some test to make sure. They did pass.

slyubomirsky · 2019-06-07T21:47:19Z

Thank you for expanding some of the comments

wweic · 2019-06-07T22:26:05Z

the lowest_bound is (1, 1), (0, 1), then (2, 1) is checked and found that it does not decrease any argument.

@MarisaKirisame Got it. I missed that you compare timestamp with historical minimum.

wweic · 2019-06-07T22:28:24Z

src/relay/pass/partial_eval.cc

+            return recurse();
+          } else {
+            // We check to see that at least one argument decrease
+            // with respect to all previous invocation.


Maybe expand the comment here that the number of recursive inline calls is bounded by the first created time of all arguments(product of created times).

wweic

It would be good to comment how the number of recursive inline is bounded by created time.

wweic · 2019-06-08T00:38:57Z

src/relay/pass/partial_eval.cc

+            /* We check to see that at least one argument decrease
+             * with respect to all previous invocation.
+             * The depth of the recursion is bounded by
+             * the sum of the time of all argument at the first call.


I think it's product in stead of sum?

it is sum. everytime the sum must decrease by at least 1.

yes you are right. @MarisaKirisame

save save save upstream lint remove bad changes fix build save save please the ci god Update src/relay/pass/partial_eval.cc Co-Authored-By: Wei Chen <[email protected]> save fix test ci is ANGRY fix rebase problem fix rebase add test save save comment

Stale

save save save upstream lint remove bad changes fix build save save please the ci god Update src/relay/pass/partial_eval.cc Co-Authored-By: Wei Chen <[email protected]> save fix test ci is ANGRY fix rebase problem fix rebase add test save save comment

tqchen previously requested changes Apr 16, 2019

View reviewed changes

include/tvm/relay/expr.h Outdated Show resolved Hide resolved

tqchen added status: need review status: need update need update based on feedbacks labels May 16, 2019

MarisaKirisame force-pushed the pe branch from 6fd71a3 to 680d141 Compare May 16, 2019 08:47

wweic requested changes May 17, 2019

View reviewed changes

MarisaKirisame force-pushed the pe branch from 5f0d630 to 3c0c18b Compare May 28, 2019 01:06

slyubomirsky reviewed May 31, 2019

View reviewed changes

MarisaKirisame force-pushed the pe branch from 499f851 to a5f9a50 Compare June 5, 2019 04:44

wweic reviewed Jun 7, 2019

View reviewed changes

wweic approved these changes Jun 7, 2019

View reviewed changes

wweic reviewed Jun 8, 2019

View reviewed changes

wweic approved these changes Jun 8, 2019

View reviewed changes

jroesch approved these changes Jun 14, 2019

View reviewed changes

save

0c0d186

save save save upstream lint remove bad changes fix build save save please the ci god Update src/relay/pass/partial_eval.cc Co-Authored-By: Wei Chen <[email protected]> save fix test ci is ANGRY fix rebase problem fix rebase add test save save comment

MarisaKirisame force-pushed the pe branch from f2f6eb0 to 0c0d186 Compare June 15, 2019 00:02

jroesch merged commit df88c41 into apache:master Jun 15, 2019

MarisaKirisame deleted the pe branch July 19, 2019 07:45

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] Partial Eval now Support Interprocedural optimization and has termination check. #3033

[Relay] Partial Eval now Support Interprocedural optimization and has termination check. #3033

MarisaKirisame commented Apr 16, 2019 •

edited

Loading

MarisaKirisame commented May 8, 2019

tqchen commented May 16, 2019

wweic left a comment

wweic May 17, 2019

MarisaKirisame May 17, 2019

MarisaKirisame May 17, 2019

wweic May 17, 2019

MarisaKirisame May 17, 2019

MarisaKirisame May 18, 2019

MarisaKirisame commented May 17, 2019

MarisaKirisame commented May 28, 2019

MarisaKirisame commented May 30, 2019

slyubomirsky May 31, 2019

MarisaKirisame May 31, 2019

slyubomirsky commented May 31, 2019

MarisaKirisame commented Jun 5, 2019

MarisaKirisame commented Jun 5, 2019

slyubomirsky commented Jun 5, 2019

wweic commented Jun 6, 2019

MarisaKirisame commented Jun 7, 2019 •

edited

Loading

MarisaKirisame commented Jun 7, 2019

slyubomirsky commented Jun 7, 2019

wweic commented Jun 7, 2019

wweic Jun 7, 2019

wweic left a comment

wweic Jun 8, 2019

MarisaKirisame Jun 8, 2019

wweic Jun 8, 2019

[Relay] Partial Eval now Support Interprocedural optimization and has termination check. #3033

[Relay] Partial Eval now Support Interprocedural optimization and has termination check. #3033

Conversation

MarisaKirisame commented Apr 16, 2019 • edited Loading

MarisaKirisame commented May 8, 2019

tqchen commented May 16, 2019

wweic left a 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

MarisaKirisame commented May 17, 2019

MarisaKirisame commented May 28, 2019

MarisaKirisame commented May 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky commented May 31, 2019

MarisaKirisame commented Jun 5, 2019

MarisaKirisame commented Jun 5, 2019

slyubomirsky commented Jun 5, 2019

wweic commented Jun 6, 2019

MarisaKirisame commented Jun 7, 2019 • edited Loading

MarisaKirisame commented Jun 7, 2019

slyubomirsky commented Jun 7, 2019

wweic commented Jun 7, 2019

Choose a reason for hiding this comment

wweic left a 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 Apr 16, 2019 •

edited

Loading

MarisaKirisame commented Jun 7, 2019 •

edited

Loading