[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

+ 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

+ 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

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


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

+ // 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

+ 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

+ 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

 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

+ }
+ // 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

+ 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

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

- (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

+ 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 7a82c72Compare 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 254adbeCompare 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 7598178Compare October 7, 2019 17:58

yzhliu reviewed Oct 7, 2019
View reviewed changes

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

tmoreau89 reviewed Oct 8, 2019
View reviewed changes

Comment threadtests/python/relay/test_pass_alter_op_layout.py Outdated
Comment threadtests/python/relay/test_pass_alter_op_layout.py
Comment threadtests/python/relay/test_pass_alter_op_layout.py

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

yzhliu approved these changes Oct 9, 2019
View reviewed changes

Comment threadsrc/relay/op/nn/pad.cc Outdated
Comment threadsrc/relay/op/nn/pad.cc Outdated
Comment threadsrc/relay/op/nn/pad.cc Outdated

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

anijain2305 force-pushed the alter branch from 6e1702a to cda4829Compare 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 24db54eCompare 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 f7286baCompare 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:masterOct 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,

Uh oh!

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