[RELAY] Port winograd ops to relay by merrymercy · Pull Request #2356 · apache/tvm

merrymercy · 2018-12-31T12:27:27Z

Port winograd related ops to relay. Now benchmarks under apps/benchmark for nvidia gpu/arm cpu/mali gpu can be compiled by relay without performance regression.
Fix scalar and concatenate in alter_op_layout

To make alter_op_layout in TOPI support both NNVM and Relay, a new argument F is added, which can be either relay.op or nnvm.sym.

Know issues:

NCHWc in x86 backend has not been ported yet

vinx13

I found an issue that alter_op_layout doesn't work with scalar
e.g. relay.add(relay.nn.conv2d(...), relay.const(1.0)) output of conv2d will fallback to original layout because the inferred layout of constant is undef

vinx13 · 2018-12-31T13:00:47Z


    if cfg.pass_enabled("FoldConstant"):
-        func = ir_pass.fold_constant(func)
+        with _target.create("llvm"):


This new target is unnecessary because FoldConstant always creates a new llvm target

BTW, can you compile inception_v3 with relay?

I can compile inception v3 without AlterOpLayout, otherwise I got type_infer.cc:314: the function is provided too many arguments (nullptr)

merrymercy · 2019-01-03T03:31:41Z

  Array<IndexExpr> kernel_size;
  std::string data_layout;
-  std::string weight_layout;
+  std::string kernel_layout;


Purpose: rename weight_layout to kernel_layout to make relay consistent with nnvm and mxnet

merrymercy · 2019-01-03T11:29:36Z

@vinx13 That issue is because the zero dimensional layout of a scalar is not defined. Fixed by my commit 78eaf0e

zhiics · 2019-01-04T05:51:02Z


    with tophub_context:
-        func = optimize(func, params)
+        with target:


May I ask why we need to have with target here? Is it used by layout altering? It seems that constant folding is not target dependent because it always uses llvm as the target.

@zhiics alter_op_layout relies on current target to query autotvm log

@vinx13 I just saw that. Thanks for your quick response. Can we pass the target to alter layout? I am asking is because for heterogeneous compilation we will pass in multiple targets. It is probably not convenient to know which target to be with here.

Ok, I passed target as an argument and use it explicitly for target-specific passes. Alter_op_layout calls functions similar to topi compute/topi schedule and will modify the graph, so I think it is not straightforward to port it to heterogeneous compilation.

@merrymercy Thanks. It doesn't really solve the problem. But I think we can keep it like this first because it at least won't break homogeneous execution. I will think about it later in the heterogeneous pass.

jroesch · 2019-01-04T23:06:32Z

+  // NOTE: Do not check weight shape here!
+  // Different backend requires different layout to compute
+  // the batch gemm stage in winograd efficiently, but we want to
+  // make this NNVM symbol work for all backends.


Could we update this comment?

jroesch · 2019-01-04T23:08:49Z

+  };

+  for (auto new_arg : new_args) {
+    // NOTE: do not support nested tuple


I think we need to generically handle nested tuples, the handling of nested tuples has appeared in multiple places including the execution + optimization of AD.

I leave it to later PRs..

jroesch · 2019-01-04T23:09:40Z

 ##### REGISTER ALTER OP LAYOUT #####
 @conv2d_alter_layout.register(["arm_cpu"])
-def _alter_conv2d_layout_arm(attrs, inputs, tinfos):
+def _alter_conv2d_layout_arm(attrs, inputs, tinfos, F):


Could we provide a documentation comment here with information about the parameters including F?

jroesch · 2019-01-04T23:10:16Z

Overall looks good, just some minor comments.

merrymercy · 2019-01-06T02:21:19Z

comments are fixed @jroesch @zhiics
ready for merge @tqchen

srkreddy1238 · 2019-01-07T16:05:19Z

thanks @merrymercy .
This PR fixes #2382. I verified it.
Also I noted weights_layout -> kernal_layout for convolution param.

srkreddy1238

AlterOpLayout changes LGTM.

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

Since this PR fixes some bugs that other PRs are pending on. I am going to merge this in.

The proposed change of weight_layout->kernel_layout needs some further discussion(in particular, the meaning of the "kernel", does it refer to the weight, or only spatial part of the weight), but because relay has not been released as a stable branch and kernel_layout was closer to NNVMv1, I won't view this as a regression for now.

tqchen · 2019-01-07T16:47:02Z

Thanks to @merrymercy @jroesch @vinx13 @srkreddy1238 @zhiics @yzhliu

[RELAY] Port winograd ops to relay

ff05bb7

merrymercy requested review from vinx13 and yzhliu December 31, 2018 12:31

vinx13 requested changes Dec 31, 2018

View reviewed changes

merrymercy commented Jan 3, 2019

View reviewed changes

fix comment

ad96f9e

Fix layout of scalar

78eaf0e

merrymercy force-pushed the relay-winograd branch from 3eaf5d0 to 78eaf0e Compare January 3, 2019 11:48

fix concatenate

3378ea7

vinx13 approved these changes Jan 4, 2019

View reviewed changes

zhiics reviewed Jan 4, 2019

View reviewed changes

jroesch requested changes Jan 4, 2019

View reviewed changes

merrymercy added 2 commits January 5, 2019 18:11

move target & fix comments

0c78c96

fix reshape argument name

6607906

fix reshape

3dc45d8

merrymercy force-pushed the relay-winograd branch from 76e9ac4 to ebcf761 Compare January 6, 2019 23:48

fix alter op layout for intel graphics and x86

c397933

merrymercy force-pushed the relay-winograd branch from ebcf761 to c397933 Compare January 7, 2019 00:04

merrymercy mentioned this pull request Jan 7, 2019

[RELAY][PASS] AlterOpLayout bugfix to handle Tuple args properly. #2383

Closed

srkreddy1238 approved these changes Jan 7, 2019

View reviewed changes

tqchen approved these changes Jan 7, 2019

View reviewed changes

tqchen merged commit 426e3bb into apache:master Jan 7, 2019

tqchen added the status: accepted label Jan 7, 2019

zhiics pushed a commit to zhiics/tvm that referenced this pull request Jan 7, 2019

[RELAY] Port winograd ops to relay (apache#2356)

19ce4e3

merrymercy deleted the relay-winograd branch January 9, 2019 11:34

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Jan 10, 2019

[RELAY] Port winograd ops to relay (apache#2356)

ef7c33f

vinx13 mentioned this pull request Jan 11, 2019

[OPT] Low-bit Quantization #2116

Merged

ZihengJiang mentioned this pull request Feb 1, 2019

TVM 0.5 Release Note #2448

Closed

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

[RELAY] Port winograd ops to relay (apache#2356)

b1d0798

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

[RELAY] Port winograd ops to relay (apache#2356)

a8bd9b8

Conversation

merrymercy commented Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinx13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

zhiics Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhiics Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jroesch commented Jan 4, 2019

Uh oh!

merrymercy commented Jan 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srkreddy1238 commented Jan 7, 2019

Uh oh!

srkreddy1238 left a comment

Choose a reason for hiding this comment

Uh oh!

tqchen commented Jan 7, 2019

Uh oh!

tqchen commented Jan 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

merrymercy commented Dec 31, 2018 •

edited

Loading

merrymercy Jan 3, 2019 •

edited

Loading

merrymercy Jan 3, 2019 •

edited

Loading

merrymercy commented Jan 3, 2019 •

edited

Loading

zhiics Jan 4, 2019 •

edited

Loading

zhiics Jan 4, 2019 •

edited

Loading

merrymercy commented Jan 6, 2019 •

edited

Loading