[RELAY][PASS] add a relay pass to count #macs of a model by yidawang · Pull Request #2609 · apache/tvm

yidawang · 2019-02-16T01:35:11Z

ATT. Currently it only counts the macs of direct CONV 2D and Dense.

eqy · 2019-02-16T01:44:53Z

Currently the number of real MACs can be dependent on the algorithm used (e.g., when winograd is suitable). Do we plan to consider these cases?

yidawang · 2019-02-16T01:51:38Z

Currently the number of real MACs can be dependent on the algorithm used (e.g., when winograd is suitable). Do we plan to consider these cases?

Yes, we can gradually improve the coverage.

zhiics · 2019-02-16T03:29:02Z

src/relay/pass/mac_count.cc

+    Array<Expr> args = call_node->args;
+    CHECK(args.size() == 2)
+        << "The number of input arguments of a CONV 2D node should be 2.";
+    const auto* expr0 = args[0]->checked_type().as<TensorTypeNode>();


expr0 -> type? because it's actually a type node instead of an expr node.

zhiics · 2019-02-16T03:32:01Z

src/relay/pass/mac_count.cc

+      return 0;
+    }
+    Array<Expr> args = call_node->args;
+    CHECK(args.size() == 2)


I am not sure if we still need these checks. Are they supposed to be valid already since we have already inferred types.

I think it wouldn't hurt. This code can still run with warning if the infer type pass is not called.

zhiics · 2019-02-16T03:33:32Z

src/relay/pass/mac_count.cc

+    Array<Expr> args = call_node->args;
+    CHECK(args.size() == 2)
+        << "The number of input arguments of a Dense node should be 2.";
+    const auto* expr0 = args[0]->checked_type().as<TensorTypeNode>();


same as above, probably more descriptive name, like data_type and weight_type

zhiics · 2019-02-16T03:36:49Z

src/relay/pass/mac_count.cc

+
+  int64_t GetCartesianProd(Array<IndexExpr> arr) {
+    int64_t ret = 1;
+    for (int i = 0; i < arr.size(); i++) {


int -> size_t since arr.size() is size_t or probably just for (const auto& it : arr)

zhiics · 2019-02-16T03:38:03Z

tests/python/relay/test_pass_mac_count.py

+    dshape2 = (m, k)
+    data1 = relay.var("data1", shape=dshape1)
+    data2 = relay.var("data2", shape=dshape2)
+    gemm = relay.nn.dense(


one liner ?

zhiics · 2019-02-16T03:39:12Z

tests/python/relay/test_pass_mac_count.py

+import tvm
+from tvm import relay
+import sys
+sys.path.append('../frontend/mxnet')


Sorry, why do we need to add it to the system path?

This is to import the model_zoo from this path correctly. I am happy to change to a more elegant solution if any suggestion.

I actually suggest to remove the resnet test, as no one can verify the correctness of the test by eyeball. And later if the algorithm changes or enhanced, it will also be a pain to deal with such test.

yzhliu · 2019-02-16T23:59:28Z

src/relay/pass/mac_count.cc

+class MacCounter : private ExprVisitor {
+ public:
+  MacCounter() {
+      count_ = 0;


two space indent

yzhliu · 2019-02-17T00:06:03Z

src/relay/pass/mac_count.cc

+    const auto* data_type = args[0]->checked_type().as<TensorTypeNode>();
+    Array<IndexExpr> data_shape = data_type->shape;
+    CHECK(data_shape.size() == 4)
+        << "The dimension of an input tensor to Dense node should be 4.";


Suggested change

<< "The dimension of an input tensor to Dense node should be 4.";

<< "The dimension of an input tensor to Conv2D node should be 4.";

some non-4D conv implement also use Conv2dAttrs though... I think the dimension of input & output does not really matter.

If a non-4D conv uses Conv2dAttrs, perhaps we should pick it out, otherwise the output wouldn't make sense?

yzhliu · 2019-02-17T00:11:27Z

src/relay/pass/mac_count.cc

+      count_ += ComputeConv2DMacs(call_node);
+    } else if (IsDenseNode(call_node)) {
+      count_ += ComputeDenseMacs(call_node);
+    }


should we log somewhere to remind users the pass only calculates MAC for Conv2D and Dense?

Added a log info in the beginning of the pass.

yzhliu

@eqy @zhiics Please take a look again.

yzhliu · 2019-02-19T21:23:34Z

src/relay/pass/mac_count.cc

+    int32_t C_ind = Layout(data_layout).Indexof('C');
+    int32_t c_ind = Layout(data_layout).Indexof('c');
+    CHECK(C_ind != -1 || c_ind != -1)
+        << "There is no input channel dimension.";


C_ind must exist actually.

zhiics · 2019-02-19T21:42:29Z

LGTM

yzhliu · 2019-02-20T08:40:35Z

Thanks @yidawang @zhiics @eqy This is merged now.

tqchen · 2019-02-20T17:47:17Z

Thanks, @yidawang @zhiics @eqy @yzhliu . Some followup comments would be nice to address as a followup:

So far the code uses hard-code pattern matching, it is a bit hard to generalize when we add ops
- e.g. we need to add support for conv2d_transpose
- The attribute registration style works much better in this case: https://github.com/dmlc/tvm/blob/master/src/relay/pass/fold_scale_axis.cc#L316
Avoid the use of dynamic_cast, mainly because it requires RTTI features. In our case, static_cast and our build in generic type checking good for most cases

* add a relay pass to count #macs of a model * remove unnecessary includes * fix end-of-file issues * address review comments * remove test resnet * address more review comments * use data layout string to locate the input channel * fix bug in conv 2d output tensor checking * C must exist

yidawang · 2019-02-21T20:15:48Z

Thanks for the feedback, @tqchen

As the MAC counting is an estimation, we only take those computationally-intensive ops into account. Therefore, I don't envision that we will extend it to many ops. We can definitely switch to your suggested attribute registration style if necessary. We will add new ops as needed.

I will send out a quick PR to fix the dynamic cast issue soon.

yidawang added 3 commits February 15, 2019 17:29

add a relay pass to count #macs of a model

453fc03

remove unnecessary includes

164046b

fix end-of-file issues

ce46195

yidawang changed the title ~~add a relay pass to count #macs of a model~~ [RELAY][PASS] add a relay pass to count #macs of a model Feb 16, 2019

zhiics reviewed Feb 16, 2019

View reviewed changes

yidawang added 3 commits February 15, 2019 20:51

address review comments

2f18c5f

remove test resnet

c515ecd

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

a91ea27

yzhliu reviewed Feb 17, 2019

View reviewed changes

yidawang added 2 commits February 16, 2019 18:05

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

885ba93

address more review comments

c47dca8

icemelon added the status: need review label Feb 19, 2019

yidawang added 3 commits February 19, 2019 10:32

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

b84122a

use data layout string to locate the input channel

38beaa3

fix bug in conv 2d output tensor checking

5de327a

yzhliu reviewed Feb 19, 2019

View reviewed changes

C must exist

4ee456f

zhiics approved these changes Feb 19, 2019

View reviewed changes

yidawang added 2 commits February 19, 2019 13:58

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

5bf2f4e

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

5422405

yzhliu approved these changes Feb 20, 2019

View reviewed changes

yzhliu added status: accepted and removed status: need review labels Feb 20, 2019

yzhliu merged commit 615fd69 into apache:master Feb 20, 2019

yidawang deleted the count_macs branch February 21, 2019 20:09

yzhliu mentioned this pull request Mar 2, 2019

[DEV] TVM v0.6 Roadmap #2623

Closed

28 tasks

tqchen mentioned this pull request Nov 8, 2019

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

Closed

	<< "The dimension of an input tensor to Dense node should be 4.";
	<< "The dimension of an input tensor to Conv2D node should be 4.";

Conversation

yidawang commented Feb 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eqy commented Feb 16, 2019

Uh oh!

yidawang commented Feb 16, 2019

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!

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!

yzhliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhiics commented Feb 19, 2019

Uh oh!

yzhliu commented Feb 20, 2019

Uh oh!

tqchen commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yidawang commented Feb 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yidawang commented Feb 16, 2019 •

edited

Loading

tqchen commented Feb 20, 2019 •

edited

Loading