Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Sep 27, 2021
Merged

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

merged 4 commits into from
Sep 27, 2021

Conversation

masahi
Copy link
Member

@masahi masahi commented Sep 20, 2021 •
edited
Loading

This PR simply adds missing registrations of ReduceInferCorrectLayout, introduced in #4059, to all of reduce ops. I found that the efficientnet v2 model has many mean op, which introduces 89 layout_transform. After this PR, there is only one layout_transform (at the beginning).

@comaniac @anijain2305

comaniac
comaniac approved these changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@masahi masahi force-pushed the layout_transform_reduction branch from 52a123a to 84353c5 Compare September 21, 2021 12:49
@masahi masahi force-pushed the layout_transform_reduction branch from 84353c5 to 1088b45 Compare September 25, 2021 12:18
@masahi
Copy link
Member Author

masahi commented Sep 25, 2021

@comaniac I've updated this PR. I found a bug in ReduceInferCorrectLayout, where exclude parameter is not taken account when deciding the new reduction axes after layout transform. Moreover, ReduceInferCorrectLayout assumes that the number of inputs to a reduction op is 1, which is not the case for variance op (it takes data and mean inputs). Both are fixed and all tests now pass.

@masahi
Copy link
Member Author

masahi commented Sep 25, 2021 •
edited
Loading

@comaniac There is one thing that's very odd about what ReduceInferCorrectLayout does. When the new layout is "NCHWc", rather than computing reduction over "NCHWc" layout directly, ReduceInferCorrectLayout inserts a conversion "NCHWc -> NCHW" before reduction, and reduction is always done in NCHW (see Animesh's comment in https://github.com/apache/tvm/pull/4059/files#r332704381 and the test added in 4059). So when compiling efficientnet v2 for x86, I still get many "NCHWc -> NCHW" conversions.

There is nothing that prevents reducing over NCHWc layout (transforming axes = [1] to axes = [1, 4], for example). The only disadvantage would be when reduction is over H and W dimensions, doing reduction over NCHWc layout may be slower. What do you think? If this sounds good, I can follow up with another PR.

@comaniac
Copy link
Contributor

I wasn't involved in that PR so I'm not really sure the root cause of that decision, but what you described makes sense to me. I'll merge this PR first and you could send a follow up one to fix the NCHWc -> NCHW issue.

cc @anijain2305 @zhiics @yzhliu it would be good if any of you folks could help provide more background s.

@masahi masahi merged commit 44d5d7a into apache:main Sep 27, 2021
@masahi
Copy link
Member Author

masahi commented Sep 27, 2021

Thanks @comaniac

AndrewZhaoLuo added a commit to AndrewZhaoLuo/tvm that referenced this pull request Sep 28, 2021
@AndrewZhaoLuo
Merge branch 'main' into aluo/onnx/support-nllloss-tests
ec71db0 
* main: (37 commits)
  [ONNX] [Relay] Dynamic squeeze (apache#9095)
  [Meta Schedule][M3b] Database (apache#9061)
  [Bugfix] Add nullptr checking for `AttrStmt` with `coproc_uop_scope` attr key (apache#9123)
  [Codegen] Swap out analyzer when outlining (apache#9117)
  [CI] bash.sh, build.sh: add option to set the container name and hostname (apache#9110)
  Ensure google-mock is installed and setup (apache#9107)
  Arm(R) Ethos(TM)-U NPU TIR to CS for Conv2D (apache#8811)
  Frontend: add onnx GlobalLpPool op (apache#8845)
  [LLVM] Refactor MakeCallPacked, NFC (apache#9118)
  prevent casting handle to other types (apache#9114)
  fix annotation of tir generic (apache#9119)
  [Relay] Register layout conversion function to more reduce ops (apache#9048)
  Fix the missing `dtype` attribute of `tir.Shuffle` in Python level (apache#9131)
  add `multiply` and remove `subtract` for dnnl json runtime (apache#9120)
  relu of dnnl json runtime only support 4-dims input (apache#9122)
  [Meta Schedule][M3a] SpaceGenerator  (apache#9079)
  [TensorIR][Bugfix] Disallow fusing loops with dependency (apache#9112)
  adding Jorn to reviewers list (apache#9105)
  [BYOC] Fix incorrect conv2d padding handling of `dnnl with c source runtime` (apache#9097)
  [Frontend][TFLite] fix apache#9078 (apache#9099)
  ...
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
@masahi @ylc
[Relay] Register layout conversion function to more reduce ops (apach…
a5bfae7 
…e#9048)

* Register layout conversion function to more reduce ops

* bug fix for exclude=True case, the original code compute wrong axes

* properly handle variance op, which has two inputs

* update test expected output
@masahi masahi mentioned this pull request Oct 11, 2021
Merged
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@masahi @ylc
[Relay] Register layout conversion function to more reduce ops (apach…
b6da88c 
…e#9048)

* Register layout conversion function to more reduce ops

* bug fix for exclude=True case, the original code compute wrong axes

* properly handle variance op, which has two inputs

* update test expected output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comaniac comaniac comaniac approved these changes

@anijain2305 anijain2305 Awaiting requested review from anijain2305

@areusch areusch Awaiting requested review from areusch

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@jwfromm jwfromm Awaiting requested review from jwfromm

@MarisaKirisame MarisaKirisame Awaiting requested review from MarisaKirisame

@mbrookhart mbrookhart Awaiting requested review from mbrookhart

@merrymercy merrymercy Awaiting requested review from merrymercy

@slyubomirsky slyubomirsky Awaiting requested review from slyubomirsky

@tqchen tqchen Awaiting requested review from tqchen

@vinx13 vinx13 Awaiting requested review from vinx13

@wweic wweic Awaiting requested review from wweic

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@masahi @comaniac