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

[microNPU] Tweak a layout transform matrix #10763

Merged

manupak merged 3 commits into apache:main from ekalda:layout-transform-fix

Mar 30, 2022

Contributor

ekalda commented Mar 24, 2022

One of the layout transform matrices currently causes the cascader to stripe
across B16 axis (which is not allowed), so change that and deal with
the implications to the get_valid_block_configs.

Contributor Author

ekalda commented Mar 24, 2022

cc: @manupa-arm @lhutton1 @NicolaLancellotti @jacobbohlin @mbaret

ekalda force-pushed the layout-transform-fix branch from 23685d9 to 33525e6 Compare

March 25, 2022 10:57

NicolaLancellotti reviewed

View reviewed changes

src/contrib/ethosu/cascader/block_config.cc Outdated Show resolved Hide resolved

lhutton1 reviewed

View reviewed changes

Contributor

lhutton1 left a comment

Looks good @ekalda! Just some small suggestions

python/tvm/relay/backend/contrib/ethosu/te/convolution.py Outdated

    
            @@ -187,7 +187,7 @@ def conv2d_compute(
          
                      [1, 0, 0, 0, 0, 0],

                      [0, 1, 0, 0, 0, 0],

                      [0, 0, 0, 1, 0, 0],

                      [0, 0, 16, 0, 1, -16],

                      [0, 0, 0, 0, 0, ofm_channels],

Contributor

lhutton1 Mar 25, 2022

Just curious: Any reason for not making this change in binary_elementwise and unary_elementwise? Additionally it looks like the nhcwb16_to_nhwc and nhwc_to_nhcwb16 matrices are the same across the operators, should we move it to a common place like util to reduce duplication?

Contributor Author

ekalda Mar 28, 2022

Initially I didn't make that change since the old transformation matrix wasn't "harmful" in the case of these operators (it was only a problem for operators that had weights or some kind of a kernel), but I think it makes sense to unify the transform for all the NPU ops since then we can indeed easily define the layout transform matrices in one place and easily reuse them across all the TEs and also in the tests.

python/tvm/relay/backend/contrib/ethosu/te/pooling.py Outdated

    
            @@ -169,7 +169,7 @@ def pooling_compute(
          
                      [1, 0, 0, 0, 0, 0],

                      [0, 1, 0, 0, 0, 0],

                      [0, 0, 0, 1, 0, 0],

                      [0, 0, 16, 0, 1, -16],

                      [0, 0, 0, 0, 0, int(ofm_channels)],

Contributor

lhutton1 Mar 25, 2022

Is int(...) necessary here?

Contributor Author

ekalda Mar 28, 2022

Yes, since the type of ofm_channels is IntImm, so without the cast it would end up causing an error in propagator.py

ekalda force-pushed the layout-transform-fix branch from 33525e6 to b0d1ae6 Compare

March 28, 2022 10:33

Contributor Author

ekalda commented Mar 28, 2022

Thanks for the reviews @lhutton1 and @NicolaLancellotti! I've addresed the comments...

manupak reviewed

View reviewed changes

Contributor

manupak left a comment

Thanks @ekalda ! (and other reviewers) .
This looks much cleaner and I've added some suggestions to improve the docs around the matrices
(so it is clear why those numbers looks like that)

python/tvm/relay/backend/contrib/ethosu/te/common.py

		from typing import Tuple, List


		def get_layout_transform_matrices(ofm_channels: int) -> Tuple[List[List[float]], List[List[float]]]:

Contributor

manupak Mar 28, 2022

docs : Lets link to this page : https://developer.arm.com/documentation/102420/0200/Functional-description/Control-and-data-flow/Supported-memory-formats-for-feature-maps here.

Contributor Author

ekalda Mar 28, 2022

Done

python/tvm/relay/backend/contrib/ethosu/te/common.py

+                      [0, 1, 0, 0, 0],
+                      [0, 0, 0, 1 / 16, 0],
+                      [0, 0, 1, 0, 0],
+                      [0, 0, 0, 0, 16],

Contributor

manupak Mar 28, 2022

docs : might worth putting a comment to indicate that b16 axis is always going to be of fixed size of 16.

Contributor Author

ekalda Mar 28, 2022

Done

python/tvm/relay/backend/contrib/ethosu/te/common.py

+                      [1, 0, 0, 0, 0, 0],
+                      [0, 1, 0, 0, 0, 0],
+                      [0, 0, 0, 1, 0, 0],
+                      [0, 0, 0, 0, 0, ofm_channels],

Contributor

manupak Mar 28, 2022

docs : lets state that the conversion of nhwc_to_nhcwb16 is lossy (because b16 axis is fixed to 16). Therefore, to recover the original "c" of "nhwc", we need to use the original channels in the transform matrix.

Contributor Author

ekalda Mar 28, 2022

Done

ekalda force-pushed the layout-transform-fix branch from b0d1ae6 to 23ecc50 Compare

March 28, 2022 15:59

ekalda commented

View reviewed changes

Contributor Author

ekalda left a comment

Thanks for the suggestions @manupa-arm! I added documentation for the transform ops, does it make sense?

python/tvm/relay/backend/contrib/ethosu/te/common.py

		from typing import Tuple, List


		def get_layout_transform_matrices(ofm_channels: int) -> Tuple[List[List[float]], List[List[float]]]:

Contributor Author

ekalda Mar 28, 2022

Done

python/tvm/relay/backend/contrib/ethosu/te/common.py

+                      [0, 1, 0, 0, 0],
+                      [0, 0, 0, 1 / 16, 0],
+                      [0, 0, 1, 0, 0],
+                      [0, 0, 0, 0, 16],

Contributor Author

ekalda Mar 28, 2022

Done

python/tvm/relay/backend/contrib/ethosu/te/common.py

+                      [1, 0, 0, 0, 0, 0],
+                      [0, 1, 0, 0, 0, 0],
+                      [0, 0, 0, 1, 0, 0],
+                      [0, 0, 0, 0, 0, ofm_channels],

Contributor Author

ekalda Mar 28, 2022

Done

ekalda force-pushed the layout-transform-fix branch from 23ecc50 to d069c27 Compare

March 29, 2022 08:37

ekalda added 3 commits

March 29, 2022 12:50


          [microNPU] Fix layout transform matrix

63eea1e

One of the layout transforms currently causes the cascader to stripe
across B16 axis (which is not allowed), so change that and deal with
the implications to the get_valid_block_configs.

Change-Id: I04199f9f35fcc31618581567483cfb80d3b5aad2


          Reduce the duplication of layout transfrom matrices

464392c

* Change the nhcwb16_to_nhwc matrix for binary and unary elementwise
  such that it matches the other NPU ops
* Reduce the number of places where the same layout transform matrices are
  defined


          Add documentation to the layout transform matrices

df32c58

ekalda force-pushed the layout-transform-fix branch from d069c27 to df32c58 Compare

March 29, 2022 11:50

manupak approved these changes

View reviewed changes

Contributor

manupak left a comment

LGTM!

manupak merged commit d0c7c78 into apache:main

Contributor

manupak commented Mar 30, 2022

Thanks @ekalda @NicolaLancellotti @lhutton1

ekalda deleted the layout-transform-fix branch

March 30, 2022 13:47

pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request


          [microNPU] Tweak a layout transform matrix (apache#10763)

0cf18d0

* [microNPU] Fix layout transform matrix

One of the layout transforms currently causes the cascader to stripe
across B16 axis (which is not allowed), so change that and deal with
the implications to the get_valid_block_configs.

Change-Id: I04199f9f35fcc31618581567483cfb80d3b5aad2

* Reduce the duplication of layout transfrom matrices

* Change the nhcwb16_to_nhwc matrix for binary and unary elementwise
  such that it matches the other NPU ops
* Reduce the number of places where the same layout transform matrices are
  defined

* Add documentation to the layout transform matrices

driazati mentioned this pull request

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet