[Relay] Partial Evaluator do concatenate, and has better termination checker for scalar. by MarisaKirisame · Pull Request #3703 · apache/tvm

MarisaKirisame · 2019-08-03T02:57:27Z

This PR:

Fix Partial Evaluator for letrec program.
Generalize Time into arbitary Meet semilattice with finite descending chain. Now people can modify the termination checker to be more aggressive (inline more) or conservative (inline less, save more time) with little ease.
Implement the FTValue - it look into a partially static value, and if it is a positive scalar integer, use the value as fuel (allow decrease on it).
Simplify Dead Code Eliminator.
Fix an Error in DCE - the old one use a worklist which does not play well with WithRec because it will escape the scope. Now there is no concept of recursion in DCE.
Reenable a test in DCE.
Fix alpha equal for letrec program.
Fix ToGraphNormalForm for letrec program.
@jroesch @tmoreau89 @weberlo

lint some lint lint add charrnn save save save remove debug remove debug remove space refactor save rewrite dce

tmoreau89 · 2019-08-03T06:12:13Z

Thanks for the PR @MarisaKirisame, could you fix the title to "[Relay] PE inline scalar" and in the description spell out "Partial evaluation" so people can get context?

MarisaKirisame · 2019-08-03T07:12:01Z

@tmoreau89 thx! It is done.

weberlo

@MarisaKirisame This PR looks sick!

I think you forgot to change the title to "Partial evaluation" instead of "PE". Also, it looks like there's more than just scalar inlining in here. A more descriptive title might be "[Relay] Extend coverage of partial evaluation".

weberlo · 2019-08-04T20:08:31Z

src/relay/pass/partial_eval.cc

+  return Fuel(make_node<FTValueNode>(tvalue));
+}
+
+/*! \brief Initially every element has Fuel of FBottom. It is the smallest element.


I don't understand the code or lattices very well yet, but if every element starts at FBottom, and you only do meets, then wouldn't it remain FBottom forever, since the meet of a minimal element and any other element is always the minimal element?

If that's the case, then maybe this should be FTop?

Good catch! It is indeed FTop. It was initally 'finitely ascending' but that sound strange, so I flip the lattice to match that.

weberlo · 2019-08-04T20:24:49Z

src/relay/pass/partial_eval.cc

+    auto ret = Meet(f, &progress);
+    return std::make_tuple(ret, progress);
+  }
+  /*! \brief return the new Fuel, and write true only iff progress is made. */


Suggested change

/*! \brief return the new Fuel, and write true only iff progress is made. */

/*! \brief return the new Fuel, and write true iff progress is made. */

I am trying to communicate that if progress is already true, in no case it will be written false. I will rewordsmith.

weberlo · 2019-08-04T20:48:45Z

tests/python/relay/test_pass_partial_eval.py

+    f_var = Var("f")
+    f = Function([x], If(op.equal(x, const(0)), const(0), x + f_var(x - const(1))))
+    orig = run_infer_type(Let(f_var, f, f_var(const(10))))
+    assert_alpha_equal(dcpe(orig), const(55))


Why is this called test_triangle?

triangle number

can you change it to test_triangular_number?

tmoreau89 · 2019-08-03T06:14:44Z

src/relay/pass/partial_eval.cc

+
+class FuelNode;
+/*! \brief A meet-semilattice with finite descending chain.
+ * It mean that we can meet two element to get an element,


tmoreau89 · 2019-08-03T06:15:23Z

src/relay/pass/partial_eval.cc

+class FuelNode;
+/*! \brief A meet-semilattice with finite descending chain.
+ * It mean that we can meet two element to get an element,
+ * and for every element, there is only finite amount of meet before getting back the same element.


only a finite

tmoreau89 · 2019-08-03T06:15:37Z

src/relay/pass/partial_eval.cc

+ * and for every element, there is only finite amount of meet before getting back the same element.
+ *
+ * Every time we recurse, we do a meet and require that progress must be made.
+ * This make sure we do not recurse infinitely in the Partial Evaluator.


This ensures

tmoreau89 · 2019-08-05T04:40:01Z

src/relay/pass/partial_eval.cc

 Expr PostProcess(const Expr&);

-/*! \brief The base container type of Relay values. */
+/*! \brief A StaticNode contain some static data that the Partial Evaluator can use. */


…checker for scalar. (apache#3703) * save lint some lint lint add charrnn save save save remove debug remove debug remove space refactor save rewrite dce * reset files * join -> meet * lint * address review comment * wordsmith

MarisaKirisame added 4 commits August 2, 2019 19:49

save

110dc84

lint some lint lint add charrnn save save save remove debug remove debug remove space refactor save rewrite dce

reset files

f6ad329

join -> meet

99fba6d

lint

47c29a7

MarisaKirisame changed the title ~~Pe inline scalar~~ [Relay] Pe inline scalar Aug 3, 2019

MarisaKirisame changed the title ~~[Relay] Pe inline scalar~~ [Relay] PE inline scalar Aug 3, 2019

weberlo suggested changes Aug 4, 2019

View reviewed changes

address review comment

a14d098

MarisaKirisame changed the title ~~[Relay] PE inline scalar~~ [Relay] Partial Evaluator do concatenate, and has better termination checker for scalar. Aug 5, 2019

tmoreau89 reviewed Aug 5, 2019

View reviewed changes

wordsmith

a304c20

tmoreau89 approved these changes Aug 5, 2019

View reviewed changes

tmoreau89 merged commit c8654e2 into apache:master Aug 5, 2019

MarisaKirisame deleted the pe-inline-scalar branch August 6, 2019 19:53

tqchen mentioned this pull request Nov 8, 2019

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

Closed

	/! \brief return the new Fuel, and write true only iff progress is made. /
	/! \brief return the new Fuel, and write true iff progress is made. /

Conversation

MarisaKirisame commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmoreau89 commented Aug 3, 2019

Uh oh!

MarisaKirisame commented Aug 3, 2019

Uh oh!

weberlo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MarisaKirisame commented Aug 3, 2019 •

edited

Loading