[Arith] add SizeVar representing non-neg valued variable in a tensor shape #4684

yzhliu · 2020-01-11T00:04:15Z

To provide extra information for arith simplification.

This is an alternative approach for #4486

More background,

https://discuss.tvm.ai/t/discuss-embed-more-bound-information-into-var-or-expr

https://discuss.tvm.ai/t/significant-increase-in-the-amount-of-cuda-code-gen-after-migrating-indexdiv-mod-to-floordiv-mod

I haven't change tvm.var to tvm.shape_var in topi because @icemelon9 is in the middle of refactoring #4644 . We can do it later.

@tqchen @icemelon9 Please review

… shape

yzhliu · 2020-01-12T01:05:24Z

@tqchen do you have any idea how come shape_var fails to be in func arg after I rebase from upstream today?

update,
nvm. find the issue.

yzhliu · 2020-01-12T05:48:36Z

@tqchen @icemelon9 CI's green. it's ready for review.

tqchen

some initial reviews, what will happen if we pass var into the compute, is it still supported?

tqchen · 2020-01-12T20:10:57Z

include/tvm/expr.h

+ */
+class ShapeVarNode : public VarNode {
+ public:
+  static ShapeVar make(DataType dtype, std::string name_hint);


use constructor in ShapeVar, as per #4648

tqchen · 2020-01-12T20:12:07Z

python/tvm/build_module.py

@@ -309,7 +313,7 @@ def get_binds(args, compact=False, binds=None):
                arg_list.append(binds[x])
        elif isinstance(x, schedule.Buffer):
            arg_list.append(x)
-        elif isinstance(x, expr.Var):


keep the original one, as we can make ShapeVar as subclass expr.Var

tqchen · 2020-01-12T20:14:09Z

cc @ZihengJiang @Hzfengsy would be great if you can help to take a look

yzhliu · 2020-01-12T20:24:07Z

passing var into the compute still works

tqchen

some additional comments wrt to integer analysis, would be great if we can comment the additional test that covers these cases

tqchen · 2020-01-12T21:24:08Z

src/arithmetic/bound_deducer.cc

+class BoundRemover : public ExprMutator {
+ public:
+  PrimExpr Remove(const PrimExpr& e) {
+    remove_bounded_ = true;


Can you document its behavior? Is it still necessary, given bound info is no longer wrapped in the assert_

tqchen · 2020-01-12T21:26:46Z

src/arithmetic/bound_deducer.cc

@@ -297,6 +331,18 @@ void BoundDeducer::Transform() {
 void BoundDeducer::Deduce() {
  Init();
  if (!success_) return;
+
+  // Any variable appears in both expr and result,


I do not understand the following comment. Why would n/4 be simplified to 0? if n>=0 the simplification rule does not stand, if n< 4, then it is a valid simplification

for >=, current logic is to first get the lower bound of the left side then deduce for i.

I see, perhaps we can add more explanation then.

Also, would it results in an incorrect bound? or just a more strict bound(which still makes the condition holds)

For a better approach. My feeling is that we should first have a rewriting that tries to move vars into one side

I believe it is just a more strict bound.
I agree, let me have a try to see if it is feasible.

as it is also using IntervalSetEvaluator, this is also not required after having shape_var return single_pt

tqchen · 2020-01-12T21:28:48Z

src/arithmetic/int_set.cc

+      // in case the domain contains variables to be relaxed.
+      return Eval(res);
+    } else {
+      return IntervalSet(0, GetRef<ShapeVar>(op));


Shouldn't it be IntervalSet::SinglePoint(var) as in Var? This is a bit overly relaxed

fair. I'll change and verify.

tqchen · 2020-01-12T21:29:21Z

src/arithmetic/int_set.cc

@@ -519,6 +536,14 @@ class IntervalSetEvaluator :
    return set->min_value.same_as(value) && set->max_value.same_as(value);
  }

+  bool SelfBoundedVar(const IntervalSet& set,
+                      const PrimExpr& value) const {


need comment about the function name.

tqchen · 2020-01-12T21:30:44Z

src/arithmetic/int_set.cc

+    IntervalSet b = this->Eval(op->b);
+    if ((MatchPoint(a, op->a) && (MatchPoint(b, op->b) || SelfBoundedVar(b, op->b)))
+          || (SelfBoundedVar(a, op->a) && SelfBoundedVar(b, op->b))) {
+      // e.g.,


This part is a bit confusing. If the divisor is already a set(which means it is relaxed), we should not return the original var.

but in Combine<Div> such case evaluates to IntervalSet::Everything().
e.g., 4 / tvm.var() => 4 / tvm.var(), while 4 / tvm.shape_var() => (-inf, +inf)

i think this can be fixed by having shape_var return single_pt(as in the other comment)

yeah... you're right.

yzhliu · 2020-01-14T03:42:41Z

@tqchen updated per comments. do you think there could be a better name than shape_var? @icemelon9 reminded there is ShapeVar in relay, which represents an n-dim tuple I believe https://github.com/apache/incubator-tvm/blob/master/python/tvm/relay/ty.py#L145

tqchen · 2020-01-14T17:28:21Z

I don't have a better idea for name. Indeed shape could indicate a tuple rather than an integer.

We could potentially rename the relay's shape template variable for clarity as it is really type var. But we could think about a better name.

tqchen · 2020-01-14T17:28:39Z

include/tvm/expr.h

+class ShapeVar : public Var {
+ public:
+  explicit ShapeVar(ObjectPtr<Object> n) : Var(n) {}
+  TVM_DLL explicit ShapeVar(std::string name_hint = "s",


Document the constructor

yzhliu · 2020-01-14T19:01:59Z

How about TIndexVar stands for tensor index variable?

tqchen · 2020-01-14T21:23:46Z

We can ask other's thoughts about it(perhaps list a few candidates and send an rfc?). TIndex seems to be a reasonable name, although it is a bit ambiguous because people need to guess what does T mean).

yzhliu · 2020-01-14T22:07:43Z

https://discuss.tvm.ai/t/rfc-name-for-non-negative-variable

yzhliu · 2020-01-15T23:05:00Z

@tqchen I have address the comments and changed the name

tqchen

a minor nits @icemelon9 please also take another look. Nice set of improvements!

tqchen · 2020-01-15T23:05:59Z

python/tvm/api.py

@@ -192,6 +192,25 @@ def var(name="tindex", dtype=int32):
    return _api_internal._Var(name, dtype)


+def size_var(name="tindex", dtype=int32):


is tindex a good default? size?

tqchen · 2020-01-15T23:08:52Z

cc @yzhliu feel free to merge after fixing the nits. BTW, it also reminds me if we would like to bring IterVar as a sub-class of Var, right now it takes Var as a member.

yzhliu · 2020-01-16T06:08:50Z

Thanks @tqchen I'll take a look into the IndexVar and send another pr

…shape (apache#4684) * [arith] add ShapeVar representing non-neg valued variable in a tensor shape * bounder remover; deal with div in int_set differently * fix bounder_remover * migrate unittest to use shape_var * use tvm.shape_var in integration & relay tests * add test case; fix Var register * fix lint * fix lint again * add default ShapeVar visitor in Relay * fix override * fix ShapeVar visit bug * revert IntervalSet for shape_var * remove bound_remover * remove is_var; use constructor for shapevar/var instead * ShapeVar -> SizeVar; add constructor comments * shape_var -> size_var in doc * tindex -> size

yzhliu added 8 commits January 8, 2020 17:14

[arith] add ShapeVar representing non-neg valued variable in a tensor…

99df462

… shape

bounder remover; deal with div in int_set differently

dd1eb76

fix bounder_remover

1817121

migrate unittest to use shape_var

22c4549

use tvm.shape_var in integration & relay tests

ecac35a

merge from upstream

28879bf

add test case; fix Var register

1b64a54

Merge remote-tracking branch 'upstream/master' into shapevar

5a1a22c

yzhliu requested a review from tqchen January 11, 2020 00:04

yzhliu requested review from Huyuwei and Laurawly as code owners January 11, 2020 00:04

yzhliu added 2 commits January 10, 2020 16:15

fix lint

4c33a0a

fix lint again

1c8ba3d

tqchen added the status: need review label Jan 11, 2020

yzhliu added 3 commits January 11, 2020 14:16

add default ShapeVar visitor in Relay

68871b8

merge from upstream

5a0bd2d

fix override

c9bdefc

fix ShapeVar visit bug

d283e00

tqchen requested changes Jan 12, 2020

View reviewed changes

tqchen added the status: need update need update based on feedbacks label Jan 12, 2020

yzhliu added 3 commits January 13, 2020 11:32

revert IntervalSet for shape_var

5589dbf

remove bound_remover

c315f36

remove is_var; use constructor for shapevar/var instead

6cb3c7f

tqchen requested changes Jan 14, 2020

View reviewed changes

yzhliu added 2 commits January 15, 2020 13:07

ShapeVar -> SizeVar; add constructor comments

d92c861

merge from upstream

a74a7ea

yzhliu changed the title ~~[Arith] add ShapeVar representing non-neg valued variable in a tensor shape~~ [Arith] add SizeVar representing non-neg valued variable in a tensor shape Jan 15, 2020

shape_var -> size_var in doc

42ba28b

tqchen approved these changes Jan 15, 2020

View reviewed changes

yzhliu added 2 commits January 15, 2020 16:59

tindex -> size

a9d6310

Merge remote-tracking branch 'upstream/master' into shapevar

48865e7

yzhliu merged commit 3a672e3 into apache:master Jan 16, 2020

yzhliu deleted the shapevar branch January 16, 2020 06:08

yzhliu added status: accepted and removed status: need review status: need update need update based on feedbacks labels Jan 16, 2020

tqchen mentioned this pull request Jan 16, 2020

[Arith] Bound for Shape Variables #4486

Closed

yzhliu mentioned this pull request Jan 25, 2020

[submodule] Update tvm to the latest apache/mxnet#17438

Merged

tqchen mentioned this pull request Feb 8, 2020

[RFC] Lower Bound for Shape Variables #4487

Closed

ZihengJiang mentioned this pull request Sep 17, 2020

TVM v0.7 Release Note Candidate #6486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Arith] add SizeVar representing non-neg valued variable in a tensor shape #4684

[Arith] add SizeVar representing non-neg valued variable in a tensor shape #4684

yzhliu commented Jan 11, 2020 •

edited

Loading

yzhliu commented Jan 12, 2020 •

edited

Loading

yzhliu commented Jan 12, 2020

tqchen left a comment

tqchen Jan 12, 2020

tqchen Jan 12, 2020

tqchen commented Jan 12, 2020

yzhliu commented Jan 12, 2020

tqchen left a comment

tqchen Jan 12, 2020

tqchen Jan 12, 2020

yzhliu Jan 13, 2020

tqchen Jan 13, 2020

yzhliu Jan 13, 2020

yzhliu Jan 14, 2020

tqchen Jan 12, 2020

yzhliu Jan 13, 2020

tqchen Jan 12, 2020

tqchen Jan 12, 2020

yzhliu Jan 13, 2020 •

edited

Loading

tqchen Jan 13, 2020 •

edited

Loading

yzhliu Jan 13, 2020

yzhliu commented Jan 14, 2020 •

edited

Loading

tqchen commented Jan 14, 2020

tqchen Jan 14, 2020

yzhliu commented Jan 14, 2020

tqchen commented Jan 14, 2020

yzhliu commented Jan 14, 2020

yzhliu commented Jan 15, 2020

tqchen left a comment

tqchen Jan 15, 2020

tqchen commented Jan 15, 2020

yzhliu commented Jan 16, 2020

		@@ -192,6 +192,25 @@ def var(name="tindex", dtype=int32):
		return _api_internal._Var(name, dtype)


		def size_var(name="tindex", dtype=int32):

[Arith] add SizeVar representing non-neg valued variable in a tensor shape #4684

[Arith] add SizeVar representing non-neg valued variable in a tensor shape #4684

Conversation

yzhliu commented Jan 11, 2020 • edited Loading

yzhliu commented Jan 12, 2020 • edited Loading

yzhliu commented Jan 12, 2020

tqchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jan 12, 2020

yzhliu commented Jan 12, 2020

tqchen 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

yzhliu Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

tqchen Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yzhliu commented Jan 14, 2020 • edited Loading

tqchen commented Jan 14, 2020

Choose a reason for hiding this comment

yzhliu commented Jan 14, 2020

tqchen commented Jan 14, 2020

yzhliu commented Jan 14, 2020

yzhliu commented Jan 15, 2020

tqchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jan 15, 2020

yzhliu commented Jan 16, 2020

yzhliu commented Jan 11, 2020 •

edited

Loading

yzhliu commented Jan 12, 2020 •

edited

Loading

yzhliu Jan 13, 2020 •

edited

Loading

tqchen Jan 13, 2020 •

edited

Loading

yzhliu commented Jan 14, 2020 •

edited

Loading