[ARITH] Revamp IntSet #3272

tqchen · 2019-06-01T03:49:29Z

Part of #2588

This PR revamps IntSet infrastructure to use the new Analyzer API.

Keep IntervalSet, remove StrideSet for now as it is not used.
Make IntSetAnalzyer as a sub-analyzer
Adapt the existing API(no API change so far, will do in subsequent PRs)
Add testcases

tqchen · 2019-06-01T03:52:33Z

cc @sgrechanik-h @sxjscience @kazum @wweic @Hzfengsy ,

libaax93 · 2019-06-01T04:02:11Z

hello there..

tqchen · 2019-06-04T15:46:26Z

ping reviewers

wweic · 2019-06-05T23:23:36Z

src/arithmetic/analyzer.cc

@@ -77,6 +78,7 @@ bool Analyzer::CanProveGreaterEqual(const Expr& expr, int64_t lower_bound) {
    return ptr->value > lower_bound;
  }
  auto bd = this->const_int_bound(this->rewrite_simplify(expr));
+  LOG(INFO) << bd;


remove it or make it more informative.

wweic · 2019-06-05T23:28:48Z

src/arithmetic/int_set.cc

+  Expr max_value = min(a->max_value, b->max_value);
+  Expr min_value = max(a->min_value, b->min_value);
+  if ((max_value.type().is_int() || max_value.type().is_uint()) &&
+      (min_value.type().is_int() || max_value.type().is_uint()) &&


Suggested change

(min_value.type().is_int() || max_value.type().is_uint()) &&

(min_value.type().is_int() || min_value.type().is_uint()) &&

wweic · 2019-06-05T23:32:16Z

src/arithmetic/int_set.cc

+  if (a->IsEmpty()) return a;
+  if (b->IsEmpty()) return b;
+  if (a->IsSinglePoint()) {
+    // assert !b->IsSinglePoint();


remove comment

tqchen · 2019-06-07T16:29:46Z

Thanks, @wweic for catching the problems. @sgrechanik-h @kazum @sxjscience please take another look if you have time

wweic

lgtm. just some typos.

include/tvm/arithmetic.h

Co-Authored-By: Wei Chen <[email protected]>

sgrechanik-h · 2019-06-08T16:50:42Z

src/arithmetic/int_set.cc

-      return IntervalSet::make(e2, e1);
-    } else if (a.is_bounded()) {
+  if (b->IsSinglePoint()) {
+    if (is_zero(b->min_value)) return IntervalSet::SinglePoint(b->min_value);


Maybe just return b since it is {0}?

sgrechanik-h · 2019-06-08T19:06:34Z

src/arithmetic/int_set.cc

    if (is_zero(divisor)) {
      LOG(FATAL) << "Modular by zero in CombineInterval Mod";
    }
-    return IntervalSet::make(make_zero(divisor.type()), divisor - 1);
+    if (analyzer->CanProveGreaterEqual(divisor, 0)) {
+      return IntervalSet(make_zero(divisor.type()), divisor - 1);


For truncated division a % b may be negative when b > 0 if a < 0, so I guess additional condition on a is required.

@tqchen I understand that this may be quite difficult to fix right now, but I think that at least some kind of warning must be issued when a cannot be proved to be non-negative, like this:

LOG(WARNING) << "The expression " << a << " is assumed to be non-negative";

sgrechanik-h · 2019-06-08T19:10:30Z

src/arithmetic/int_set.cc

+                                    IntervalSet a,
+                                    IntervalSet b) {
+  if (a->IsSinglePoint() && b->IsSinglePoint()) {
+    return IntervalSet::SinglePoint(a->min_value % b->min_value);


sgrechanik-h · 2019-06-08T19:10:47Z

src/arithmetic/int_set.cc

+                                    IntervalSet a,
+                                    IntervalSet b) {
+  if (a->IsSinglePoint() && b->IsSinglePoint()) {
+    return IntervalSet::SinglePoint(a->min_value % b->min_value);


sgrechanik-h · 2019-06-08T19:18:56Z

src/arithmetic/int_set.cc

-    IntSet false_set = this->Eval(op->false_value);
-    return Union({false_set, true_set});
+
+  IntervalSet VisitExpr_(const Select* op) final {


Do we need the same thing for if_then_else?

unlikely we will use if_then_else for indices

sgrechanik-h · 2019-06-10T08:22:38Z

src/arithmetic/int_set.h

+ * \brief Symbolic interval set.
+ *
+ * \note We intentionally keep the internal of IntSet private,
+         as we anticipate that


This comment looks unfinished

sgrechanik-h · 2019-06-10T08:29:41Z

src/arithmetic/int_set.h

+  /*! \return whether interval represent nothing */
+  bool IsEmpty() const {
+    // during computations, either extreme could occur.
+    return is_pos_inf(min_value) || is_neg_inf(max_value);


Shouldn't we also try something like CanProve(min_value > max_value)?

Was handled in the simplification logic of intersect, but could debate where we should add it

Such an empty set may also be created directly with a constructor, so it may be worth adding the check into the constructor. However this may be a rare case, so I don't insist.

derisavi · 2019-06-10T19:29:30Z

src/arithmetic/int_set.cc

-  LOG(WARNING) << "Return Everything in CombineInterval Mul";
-  return IntSet::everything();
+  DLOG(WARNING) << "Return Everything in CombineInterval Mul";
+  return IntervalSet::Everything();


Seems to me that we are not handling this case: b is not a single point but we can still prove it's always non-negative or always non-positive.

The same statement applies to Combine<Div> and possibly Combine<Mod> below.

true, pushing for this direction might involve more thoughts(and related use-cases) let us do it in a follow up

fine with me to do it a follow-up. Regarding use-cases: It's sometimes hard to come up with a realistic use-case. On the other hand, when there is a missed opt opportunity, it will be time-consuming to find out that it was because of a missed rule in some fundamental infra like this.

In summary, I suggest to do it in a follow-up without worrying about a real use-case.

derisavi · 2019-06-10T19:35:13Z

src/arithmetic/int_set.cc

+
+  IntervalSet VisitExpr_(const Select* op) final {
+    IntervalSet true_set = this->Eval(op->true_value);
+    IntervalSet false_set = this->Eval(op->false_value);


Is it possible to evaluate op->true_value and op->false_value under the contexts of op->condition and NOT op->condition respectively?

I think we might, but need to put more thoughts into this, and check if there is any particular usecase of interest

I can think of this use case: message passing during InferBound where we have Select or if_then_else in the lambda expression (e.g., padding in conv, maxpool, etc.)

tqchen · 2019-06-10T23:38:08Z

Thanks, @sgrechanik-h @derisavi for helpful comments! please take another look when you have time.

sgrechanik-h · 2019-06-13T13:42:35Z

src/arithmetic/int_set.cc

+
+// Range related code
+inline bool ProveEqual(Expr lhs, Expr rhs) {
+  return is_zero(ir::Simplify(lhs - rhs));


By the way, I guess ir::Simplify is the Halide simplifier. Can it be replaced we the new rewrite or canonical simplifiers in this file?

will do that in a follow up PR

tqchen · 2019-06-13T20:10:35Z

Thanks, @derisavi @sgrechanik-h @wweic @Hzfengsy . this PR is now merged. Let us address the remaining comments in the subsequent PRs

tqchen added the status: need review label Jun 1, 2019

tqchen force-pushed the arith branch from ea7343c to 145b7a2 Compare June 1, 2019 03:51

tqchen force-pushed the arith branch from 145b7a2 to 01b31fc Compare June 1, 2019 04:08

[ARITH] Revamp IntSet

0e5701f

tqchen force-pushed the arith branch from 01b31fc to 0e5701f Compare June 2, 2019 16:47

Hzfengsy approved these changes Jun 5, 2019

View reviewed changes

wweic requested changes Jun 5, 2019

View reviewed changes

remove additional log

a037c77

tqchen force-pushed the arith branch from 91b4342 to a037c77 Compare June 7, 2019 16:29

wweic approved these changes Jun 7, 2019

View reviewed changes

include/tvm/arithmetic.h Outdated Show resolved Hide resolved

include/tvm/arithmetic.h Outdated Show resolved Hide resolved

include/tvm/arithmetic.h Outdated Show resolved Hide resolved

include/tvm/arithmetic.h Outdated Show resolved Hide resolved

tqchen and others added 4 commits June 8, 2019 09:19

Update include/tvm/arithmetic.h

37dee24

Co-Authored-By: Wei Chen <[email protected]>

Update include/tvm/arithmetic.h

84997d2

Co-Authored-By: Wei Chen <[email protected]>

Update include/tvm/arithmetic.h

79ed92e

Co-Authored-By: Wei Chen <[email protected]>

Update include/tvm/arithmetic.h

50c0238

Co-Authored-By: Wei Chen <[email protected]>

sgrechanik-h reviewed Jun 8, 2019

View reviewed changes

sgrechanik-h reviewed Jun 10, 2019

View reviewed changes

derisavi reviewed Jun 10, 2019

View reviewed changes

fix review comments

acf276b

Fix

49767d6

tqchen force-pushed the arith branch from 3769dd6 to 49767d6 Compare June 12, 2019 21:55

sgrechanik-h reviewed Jun 13, 2019

View reviewed changes

tqchen added 2 commits June 13, 2019 08:21

assumes positive a for now

e4db784

ok

55dc610

tqchen merged commit 153417a into apache:master Jun 13, 2019

tqchen added status: accepted and removed status: need review labels Jun 13, 2019

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

[ARITH] Revamp IntSet (apache#3272)

827a8fb

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

[ARITH] Revamp IntSet (apache#3272)

f80aacf

kevinthesun mentioned this pull request Jun 30, 2019

[Performance regression] Revamp IntSet #3272 causing GluonCV SSD performance issue #3465

Closed

merrymercy mentioned this pull request Jul 24, 2019

[TensorIR] Add GPU support tlc-pack/tvm-tensorir#2

Merged

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] Revamp IntSet #3272

[ARITH] Revamp IntSet #3272

tqchen commented Jun 1, 2019 •

edited

Loading

tqchen commented Jun 1, 2019

libaax93 commented Jun 1, 2019

tqchen commented Jun 4, 2019

wweic Jun 5, 2019

wweic Jun 5, 2019

wweic Jun 5, 2019

tqchen commented Jun 7, 2019

wweic left a comment

sgrechanik-h Jun 8, 2019

sgrechanik-h Jun 8, 2019

sgrechanik-h Jun 15, 2019

sgrechanik-h Jun 8, 2019

sgrechanik-h Jun 8, 2019

sgrechanik-h Jun 8, 2019

tqchen Jun 10, 2019

sgrechanik-h Jun 10, 2019

sgrechanik-h Jun 10, 2019

tqchen Jun 10, 2019

sgrechanik-h Jun 11, 2019

derisavi Jun 10, 2019 •

edited

Loading

tqchen Jun 10, 2019

derisavi Jun 12, 2019

derisavi Jun 10, 2019

tqchen Jun 10, 2019

derisavi Jun 12, 2019

tqchen commented Jun 10, 2019

sgrechanik-h Jun 13, 2019

tqchen Jun 13, 2019

tqchen commented Jun 13, 2019

	(min_value.type().is_int() \|\| max_value.type().is_uint()) &&
	(min_value.type().is_int() \|\| min_value.type().is_uint()) &&

[ARITH] Revamp IntSet #3272

[ARITH] Revamp IntSet #3272

Conversation

tqchen commented Jun 1, 2019 • edited Loading

tqchen commented Jun 1, 2019

libaax93 commented Jun 1, 2019

tqchen commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jun 7, 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

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

derisavi Jun 10, 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

tqchen commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jun 13, 2019

tqchen commented Jun 1, 2019 •

edited

Loading

derisavi Jun 10, 2019 •

edited

Loading