[ARITH] Analyzer RewriteSimplifier: add/sub/mul/div/mod #2722

tqchen · 2019-03-04T05:26:07Z

This PR contains one step of #2588

RewriteSimplifier Infra
Rules for add/sub/mul/div/mod
Test-cases covering the rules.

We still need to add rules for other ops. Given that these rules are self-contained and covered by test-case, I will request reviews and aim to merge them in before we can in parallel add support for other ops.

Need a few reviewers to check since the rules can be tricky(even though when each of them are covered by at least one test case)

tqchen · 2019-03-04T05:26:36Z

cc @sgrechanik-h @icemelon9 @merrymercy @kazum @grwlf @ZihengJiang @wweic @junrushao1994 @MarisaKirisame please help review the PR

merrymercy · 2019-03-04T15:59:35Z

This fails for the case in #1711 (comment)

ck.verify(x/6*6 + (((x/3) % 2)*3) + (x % 3), x)

tqchen · 2019-03-04T18:49:14Z

@merrymercy Rewrite simplifier is hard for solving cases like the nested fusion. We will need a improved canonical simplifier to deal with that. So that specific case will need to wait for a better canonical simplifier

src/arithmetic/rewrite_simplify.cc

MarisaKirisame · 2019-03-05T00:11:03Z

src/arithmetic/rewrite_simplify.cc

+  // Recursive rewrite x
+  // we limit maximum number of recursive rewrite allowed to
+  // avoid infinite loop
+  Expr RecursiveRewrite(Expr x) {


const Expr&

kazum · 2019-03-05T08:07:29Z

The following case succeeds.

ck.verify((x + 1) + x, x * 2 + 1);

However, the following case fails. Is this out of scope of this PR?

ck.verify(x + (x + 1), x * 2 + 1);

sgrechanik-h

Great work. I tried to enable it in my autodiff alongside with other simplifiers and didn't get any errors (and there were some small improvements). My main concern is that it is very hard to be sure about the correctness, maybe we should consider testing by generating random expressions using hypothesis or something like this?

sgrechanik-h · 2019-03-05T08:31:34Z

src/arithmetic/pattern_match.h

  }

 private:
  typename TA::Nested a_;
  typename TB::Nested b_;
 };

+template<typename TA>
+class PConstWithTypeLike :
+      public Pattern<PConstWithTypeLike<TA> > {


Please add a class description and some description for its constructor parameters.

sgrechanik-h · 2019-03-05T09:29:47Z

src/arithmetic/rewrite_simplify.cc

+  Expr RecursiveRewrite(Expr x) {
+    if (recur_counter_ >= kMaxRecurCount) return x;
+    ++recur_counter_;
+    return Mutate(x);


I have some doubts about this mechanism. Since the counter is global (for the whole expression), we might get into a situation when we hit the limit of recursive rewrites in one part of the expression and there won't be any more recursive steps allowed for other parts of the expression. Moreover, I think the limit should grow linearly with the expression size instead of being fixed. I would suggest some kind of fuel system, however I'm not sure if it's worth the trouble.

Also we might want to give warnings about expressions which cause the simplifier to exhaust the number of allowed recursive rewrites.

I thought it's was depth. Looks like it's number of calls we can make for the whole expression. Doesn't it make more sense to limit depth of RecursiveRewrite calls?

sgrechanik-h · 2019-03-05T10:40:02Z

src/arithmetic/rewrite_simplify.cc

+    TVM_TRY_REWRITE(x * y - x * z, x * (y - z));
+    TVM_TRY_REWRITE(y * x - x * z, x * (y - z));
+    TVM_TRY_REWRITE(x * y - z * x, x * (y - z));
+    TVM_TRY_REWRITE(y * x - z * x, x * (y - z));


There many rules which are the same up to commutativity. Maybe we should consider adding some option to the Match function to allow matching up to commutativity?

sergei-mironov · 2019-03-05T20:29:19Z

src/arithmetic/rewrite_simplify.cc

+
+  if (IsIndexType(op->type)) {
+    // constant simplification rule
+    TVM_TRY_REWRITE((x + c1) * c2, x * c2 + c1 * c2);


Does this rule contradict with one of TVM_TRY_REWRITE(x * y + x * z, x * (y + z)) from the Add section? Does RewriteSimplifier stabilize if repeatedly applied many times?

Edit: (I'm not an expert in rewriting) Do we have some principle or good theory when specifying rewriting rules? For example, could we require that the result of rewriting always has less nodes than the input? I think we should better describe the current approach in comments.

This rewrite only works for constant and note that c1 * c2 will get automatically constant folded

sergei-mironov · 2019-03-05T20:50:30Z

src/arithmetic/rewrite_simplify.cc

+namespace arith {
+
+using namespace ir;
+


In my opinion, this approach to specifying rewriting rules has pros and cons. The good thing is that rule definition looks really clear. The bad thing I see is that rules are declared one after another in a single C++ function. We can't combine rules into bigger rules, and, more importantly, we can't call them individually e.g. in order to apply a testcase.

in theory, CSE(common subexpression elimination) in compiler could pull out some of the common computed element of these rules and not recompute them

tqchen · 2019-03-05T22:40:08Z

Thanks, @kazum @merrymercy @grwlf @sgrechanik-h for reviews, to collectivly answer some of the questions

Coverage of Testcases

We do not expect RewriteSimplifier to simplify all the possible cases. As a matter of fact, because most rewriting are local it is hard to get simplification over a long distance. This is a known limitation of the rewrite-based simplification.

To complement that, we will need a new simplification strategy: CanonicalSimplifier, which brings canonical form and will make it much easier to simplify cases like (x + y + z + x)=> x*2 + y + z. However, there are cases that are covered by RewriteSimplifier which may not be covered by CanonicalSimplifiers, in particular, the rules wrt to max/mins.

So the goal of this PR is to get something that is correct and off the ground. We will continue to bring in new CanonicalSimplifier and improve both collectively.

Correctness

Correctness is a big problem for us, that is why we need test-coverage, and code reviews to review these rules. The general goal is to vet the rules explicitly. Again since many of the weights are going to be carried by CanonicalSimplifier, we do not need to have a comprehensive list of rules in here.

wweic · 2019-03-05T20:56:25Z

src/arithmetic/rewrite_simplify.cc

+  Expr RecursiveRewrite(Expr x) {
+    if (recur_counter_ >= kMaxRecurCount) return x;
+    ++recur_counter_;
+    return Mutate(x);


I thought it's was depth. Looks like it's number of calls we can make for the whole expression. Doesn't it make more sense to limit depth of RecursiveRewrite calls?

wweic · 2019-03-05T20:57:47Z

src/arithmetic/rewrite_simplify.cc

+    TVM_TRY_REWRITE(max(x, y - z) + z, max(x + z, y));
+    TVM_TRY_REWRITE(max(x - z, y) + z, max(x, y + z));
+    TVM_TRY_REWRITE(max(x, y) + min(x, y), x + y);
+    TVM_TRY_REWRITE(min(x, y) + max(x, y), x + y);


Also add the following rule?

TVM_TRY_REWRITE(max(x, y) + min(y, x), x + y); TVM_TRY_REWRITE(min(x, y) + max(y, x), x + y);

kazum · 2019-03-07T09:11:58Z

src/arithmetic/rewrite_simplify.cc

+  TVM_TRY_REWRITE(select(x, y, z) - z,
+                  select(x, y - z, ZeroWithTypeLike(z)));
+  TVM_TRY_REWRITE(select(x, y, z) - y,
+                  select(x, ZeroWithTypeLike(y), z - y));


Looks like tests for these cases are missing.

ck.verify(tvm.expr.Select(x > 0, y, z) - y, tvm.expr.Select(x > 0, 0, z - y)) ck.verify(tvm.expr.Select(x > 0, y, z) - z, tvm.expr.Select(x > 0, y - z, 0))

tqchen · 2019-03-09T03:16:25Z

Changed to limit recursive depth as per suggestion by @wweic and @sgrechanik-h , added test cases suggested by @kazum . Please help take another look

tqchen · 2019-03-10T16:31:47Z

Thanks @kazum @wweic @sgrechanik-h @grwlf , this is now merged

tqchen · 2019-03-10T21:22:15Z

Follow up PR #2768

tqchen added the status: need review label Mar 4, 2019

tqchen force-pushed the rewrite branch 2 times, most recently from f590122 to 4ffb199 Compare March 4, 2019 05:34

MarisaKirisame reviewed Mar 5, 2019

View reviewed changes

sgrechanik-h approved these changes Mar 5, 2019

View reviewed changes

sergei-mironov reviewed Mar 5, 2019

View reviewed changes

wweic reviewed Mar 6, 2019

View reviewed changes

kazum reviewed Mar 7, 2019

View reviewed changes

[ARITH] RewriteSimplifier: add/sub/mul/div/mod

82a9385

tqchen force-pushed the rewrite branch from 4ffb199 to 794b247 Compare March 9, 2019 03:15

Change to recursive depth limit. address reviews

d88051b

tqchen force-pushed the rewrite branch from 794b247 to d88051b Compare March 9, 2019 04:27

tqchen merged commit ec95675 into apache:master Mar 10, 2019

tqchen removed the status: need review label Mar 10, 2019

tqchen mentioned this pull request Mar 10, 2019

[ARITH] RewriteSimplifier: min/max, logical, select #2768

Merged

2 tasks

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

[ARITH] Analyzer RewriteSimplifier: add/sub/mul/div/mod (apache#2722)

c8eb7d9

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

[ARITH] Analyzer RewriteSimplifier: add/sub/mul/div/mod (apache#2722)

e12a8dc

tqchen deleted the rewrite branch July 2, 2019 16:33

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

[ARITH] Analyzer RewriteSimplifier: add/sub/mul/div/mod #2722

[ARITH] Analyzer RewriteSimplifier: add/sub/mul/div/mod #2722

tqchen commented Mar 4, 2019

tqchen commented Mar 4, 2019 •

edited

Loading

merrymercy commented Mar 4, 2019

tqchen commented Mar 4, 2019

MarisaKirisame Mar 5, 2019

kazum commented Mar 5, 2019

sgrechanik-h left a comment

sgrechanik-h Mar 5, 2019

sgrechanik-h Mar 5, 2019

wweic Mar 5, 2019

sgrechanik-h Mar 5, 2019

sergei-mironov Mar 5, 2019 •

edited

Loading

tqchen Mar 5, 2019

sergei-mironov Mar 5, 2019 •

edited

Loading

tqchen Mar 5, 2019

tqchen commented Mar 5, 2019

wweic Mar 5, 2019

wweic Mar 5, 2019

kazum Mar 7, 2019

tqchen commented Mar 9, 2019

tqchen commented Mar 10, 2019

tqchen commented Mar 10, 2019 •

edited

Loading

[ARITH] Analyzer RewriteSimplifier: add/sub/mul/div/mod #2722

[ARITH] Analyzer RewriteSimplifier: add/sub/mul/div/mod #2722

Conversation

tqchen commented Mar 4, 2019

tqchen commented Mar 4, 2019 • edited Loading

merrymercy commented Mar 4, 2019

tqchen commented Mar 4, 2019

Choose a reason for hiding this comment

kazum commented Mar 5, 2019

sgrechanik-h 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

sergei-mironov Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-mironov Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Mar 5, 2019

Coverage of Testcases

Correctness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Mar 9, 2019

tqchen commented Mar 10, 2019

tqchen commented Mar 10, 2019 • edited Loading

tqchen commented Mar 4, 2019 •

edited

Loading

sergei-mironov Mar 5, 2019 •

edited

Loading

sergei-mironov Mar 5, 2019 •

edited

Loading

tqchen commented Mar 10, 2019 •

edited

Loading