[Relay][AlterOp] NHWC to NCHWc support for Pool, concatenate, sum. by anijain2305 · Pull Request #4059 · apache/tvm

anijain2305 · 2019-10-04T22:34:55Z

For quantized InceptionV3, the number of layout transforms are reduced with this PR

Original - 260 Layout transforms / 1836 total operators
After PR - 172 Layout transforms / 1718 total operators
(Many of the transforms out of 172 are cheaper than the original 260 LT).

anijain2305 · 2019-10-05T22:50:35Z

@yzhliu @vinx13 @ZihengJiang @tqchen Please review when you get time.

yzhliu · 2019-10-07T00:16:24Z

src/relay/op/nn/pad.cc

+  bool is_layout_modified = new_in_layouts.defined();
+  if (new_in_layouts.defined()) {
+    LOG(INFO) << "New = " << new_in_layouts[0];
+    LOG(INFO) << "Old = " << old_in_layouts[0];


yzhliu · 2019-10-07T00:16:58Z

src/relay/op/nn/pad.cc

+        new_pad_width.push_back(axis_pad_width.at(axis_name));
+      } else {
+        // This is the axis that got split. So, check that pad_width was [0, 0] originally.
+        LOG(INFO) << "Old axis " << axis_name;


yzhliu · 2019-10-07T00:18:57Z

src/relay/op/nn/pad.cc

+      ret = Layout::Undef();
+    }
+  }
+  LOG(INFO) << "Final layout = " << ret;


yzhliu · 2019-10-07T19:55:56Z

src/relay/op/nn/pad.cc

+    // split.
+
+    // 1) Create a map from axis to param_width using old layout.
+    std::map<std::string, tvm::Array<tvm::Expr>> axis_pad_width;


do you think it is more convenient to add a hash function in LayoutAxis, and use const LayoutAxis& as key?

yzhliu · 2019-10-07T19:58:54Z

src/relay/op/nn/pad.cc

+    std::map<std::string, tvm::Array<tvm::Expr>> axis_pad_width;
+    int index_counter = 0;
+    CHECK_EQ(new_in_layouts.size(), 1);
+    for (auto axis : old_in_layouts[0]->axes) {


CHECK_EQ(old_in_layouts[0]->axes.size(), params->pad_width.size())

yzhliu · 2019-10-07T22:06:15Z

src/relay/op/nn/pad.cc

+    for (auto axis : new_in_layouts[0]->axes) {
+      auto axis_name = axis->var->name_hint;
+      if (axis_pad_width.count(axis_name) != 0) {
+        // This is primal axis. So, directly use the original pad_width.


there's no guarantee that the old layouts are all primal, right?

yzhliu · 2019-10-07T22:21:13Z

src/relay/op/tensor/reduce.cc

  return r_axes;
 }

+Array<Array<Layout>> SumInferCorrectLayout(const Attrs& attrs, const Array<Layout>& new_in_layouts,


Suggested change

Array<Array<Layout>> SumInferCorrectLayout(const Attrs& attrs, const Array<Layout>& new_in_layouts,

Array<Array<Layout>> ReduceInferCorrectLayout(const Attrs& attrs, const Array<Layout>& new_in_layouts,

yzhliu · 2019-10-07T22:32:56Z

src/relay/op/tensor/reduce.cc

+      }
+      // Collect only the primal axis.
+      if (LayoutAxis::Get(layout_axis).IsPrimal()) {
+        new_layout_string += layout_dim;


how about the sub-axis?

Sub axis, we dont have to do anything. Basically, this will insert a layout transform from NCHWC16c to NCHW before reduce.

yzhliu · 2019-10-07T22:37:43Z

src/relay/op/tensor/reduce.cc

+    std::string new_layout_string = "";
+    int axis_index = 0;
+    for (auto layout_axis : new_in_layouts[0]->axes) {
+      char layout_dim = layout_axis->var->name_hint.at(0);


would be better to consistently use LayoutAxis

tests/python/relay/test_pass_alter_op_layout.py

src/relay/op/nn/pad.cc

yzhliu · 2019-10-10T05:58:42Z

@anijain2305 Could you take a look at the failure test?

anijain2305 · 2019-10-11T00:46:42Z

I am getting some coreml flaky errors with pad operator. I will try to reproduce them. Currently, removing Pad changes from this PR. Will send separately.

anijain2305 · 2019-10-11T03:36:38Z

@yzhliu @zhiics can we get this merged? I will send a PR for pad separately.

zhiics · 2019-10-11T03:38:18Z

I will TAL later.

zhiics

Only a few small questions to clarify. It looks good to me.

zhiics · 2019-10-11T04:25:22Z

python/tvm/relay/frontend/tflite.py

-                                                          (pad_top, pad_bottom),
-                                                          (pad_left, pad_right),
-                                                          (0, 0)))
+            do_pad = not (pad_top == 0 and pad_bottom == 0 and pad_left == 0 and pad_right == 0)


Is this needed? or should it be done with pad?

This one is fine. It is just an optimization that ensures that we do not redundant pads.

zhiics · 2019-10-11T05:11:03Z

src/relay/op/tensor/reduce.cc

+                                              const Array<Layout>& old_in_layouts,
+                                              const Array<Array<IndexExpr>>& old_in_shapes) {
+  // NOTE: Discard "const" qualifier here.
+  ReduceAttrs* params = const_cast<ReduceAttrs*>(attrs.as<ReduceAttrs>());


Why const_cast here? is it because we need to change the axis?

zhiics

LGTM

zhiics · 2019-10-11T05:57:14Z

Thanks @anijain2305 @yzhliu @tmoreau89. This is now merged.

…m. (apache#4059)

anijain2305 force-pushed the alter branch from 2baa1e0 to 7a82c72 Compare October 5, 2019 22:48

anijain2305 changed the title ~~[Relay][AlterOp] WIP - NHWC to NCHWC16c support for Pool, pad, concatenate.~~ [Relay][AlterOp] NHWC to NCHWC16c support for Pool, pad, concatenate. Oct 5, 2019

anijain2305 changed the title ~~[Relay][AlterOp] NHWC to NCHWC16c support for Pool, pad, concatenate.~~ [Relay][AlterOp] NHWC to NCHWc support for Pool, pad, concatenate. Oct 5, 2019

kevinthesun added the status: need review label Oct 6, 2019

anijain2305 force-pushed the alter branch 6 times, most recently from 2d2794e to 254adbe Compare October 7, 2019 17:18

anijain2305 changed the title ~~[Relay][AlterOp] NHWC to NCHWc support for Pool, pad, concatenate.~~ [Relay][AlterOp] NHWC to NCHWc support for Pool, pad, concatenate, sum. Oct 7, 2019

anijain2305 force-pushed the alter branch from 254adbe to 7598178 Compare October 7, 2019 17:58

yzhliu reviewed Oct 7, 2019

View reviewed changes

anijain2305 force-pushed the alter branch from 7598178 to 3ddc122 Compare October 8, 2019 06:51

tmoreau89 reviewed Oct 8, 2019

View reviewed changes

tests/python/relay/test_pass_alter_op_layout.py Outdated Show resolved Hide resolved

tests/python/relay/test_pass_alter_op_layout.py Show resolved Hide resolved

tests/python/relay/test_pass_alter_op_layout.py Show resolved Hide resolved

anijain2305 force-pushed the alter branch 4 times, most recently from c65a536 to 87f012e Compare October 9, 2019 00:35

yzhliu approved these changes Oct 9, 2019

View reviewed changes

src/relay/op/nn/pad.cc Outdated Show resolved Hide resolved

src/relay/op/nn/pad.cc Outdated Show resolved Hide resolved

src/relay/op/nn/pad.cc Outdated Show resolved Hide resolved

anijain2305 force-pushed the alter branch from 87f012e to 6e1702a Compare October 9, 2019 19:13

anijain2305 force-pushed the alter branch from 6e1702a to cda4829 Compare October 10, 2019 16:38

tqchen added the status: need update need update based on feedbacks label Oct 10, 2019

anijain2305 force-pushed the alter branch from cda4829 to 24db54e Compare October 11, 2019 00:44

[Relay][AlterOp] NHWC to NCHWc support for Pool, pad, concatenate, sum.

f7286ba

anijain2305 force-pushed the alter branch from 24db54e to f7286ba Compare October 11, 2019 00:46

zhiics changed the title ~~[Relay][AlterOp] NHWC to NCHWc support for Pool, pad, concatenate, sum.~~ [Relay][AlterOp] NHWC to NCHWc support for Pool, concatenate, sum. Oct 11, 2019

zhiics reviewed Oct 11, 2019

View reviewed changes

zhiics approved these changes Oct 11, 2019

View reviewed changes

zhiics merged commit d69c6fd into apache:master Oct 11, 2019

zhiics added status: accepted and removed status: need review status: need update need update based on feedbacks labels Oct 11, 2019

anijain2305 mentioned this pull request Oct 11, 2019

[Relay][AlterOpLayout] NHWC to NCHWc pad operator. #4103

Merged

anijain2305 added a commit to anijain2305/tvm that referenced this pull request Oct 17, 2019

[Relay][AlterOp] NHWC to NCHWc support for Pool, pad, concatenate, su…

dcd876d

…m. (apache#4059)

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

[Relay][AlterOp] NHWC to NCHWc support for Pool, pad, concatenate, su…

8d7d2ad

…m. (apache#4059)

tqchen mentioned this pull request Nov 8, 2019

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

Closed

masahi mentioned this pull request Sep 20, 2021

[Relay] Register layout conversion function to more reduce ops #9048

Merged

	Array<Array<Layout>> SumInferCorrectLayout(const Attrs& attrs, const Array<Layout>& new_in_layouts,
	Array<Array<Layout>> ReduceInferCorrectLayout(const Attrs& attrs, const Array<Layout>& new_in_layouts,

Conversation

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

Uh oh!

Uh oh!

anijain2305 commented Oct 5, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yzhliu commented Oct 10, 2019

Uh oh!

anijain2305 commented Oct 11, 2019

Uh oh!

anijain2305 commented Oct 11, 2019

Uh oh!

zhiics commented Oct 11, 2019

Uh oh!

zhiics left a comment

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!

zhiics left a comment

Choose a reason for hiding this comment

Uh oh!

zhiics commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anijain2305 commented Oct 4, 2019 •

edited

Loading