You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/10/20 10:22:38 UTC
[GitHub] [incubator-tvm] cbalint13 opened a new pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
cbalint13 opened a new pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714
This small PR enable flexible parametrizations for ```conv2d_NCHWc_int8()``` int terms of ```int32_lanes``` and ```int8_elements``` .
-----
**Goal:**
* Allow ```conv2d_NCHWc_int8()``` to be called with any custom values of ```int32_lanes``` and ```int8_elements```
* Such flexibility (custom lane&elements) will be desired for future customizable ISA (will publish soon such arch contrib).
**Changes:**
1. Make ```n_elems``` (default=4) **optional parameter** instead of static ```n_elems = 4``` hardcode.
2. Default **fallback config** now emit valid configuration for cases when```int32_lane < int8_num_elements``` too.
3. All **old behavior is kept**, so actual Intel or ARM specific intrinsic will benefit the same default values as before.
@anijain2305 @zhiics @vinx13 @ZihengJiang please help me with the review,
Thank You !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] giuseros commented on pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#issuecomment-719473248
Thanks for the explanation and for the pointers @cbalint13 ! This LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] cbalint13 commented on a change in pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on a change in pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#discussion_r514604768
##########
File path: python/tvm/topi/generic/conv2d.py
##########
@@ -51,7 +51,7 @@ def fallback_schedule_cpu_common_int8(cfg, wkl, int32_lanes, num_int8_elements):
num_int8_elements,
)
- oc_bn = int32_lanes
+ oc_bn = int32_lanes if int32_lanes >= num_int8_elements else num_int8_elements
Review comment:
@giuseros,
It is "vector size" of a specific machine, but can vary from "machine" to "machine".
* From HW point ```lanes``` are registers (N x 32bit) where data products ```num_int8``` accumulates after multiplications.
* I.E. case of ARM & Intel, the design is: 16 lanes (16 registers) where 4 multiplies (instant ones) accumulates into each.
Now e.g. MARLANN uses 2 lanes, each one accumulating 8 multiplies, see: [ISA](https://github.com/SymbioticEDA/MARLANN/blob/master/docs/isa.md)
Furthermore, working on a more generic HW accelerator where design may be parametrizable: ```lanes```, ```macs```.
**Goal:** would like to reuse existing TVM routines as templates, but without the hard coded lanes/macs .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] cbalint13 commented on a change in pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on a change in pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#discussion_r514608916
##########
File path: python/tvm/topi/generic/conv2d.py
##########
@@ -180,7 +180,7 @@ def schedule_conv_NCHWc_cpu_common_int8(
ow_chunk, ow_block = s[CC].split(ow, factor=reg_n)
assert oc_bn % int32_lanes == 0
- assert ic_bn % 4 == 0 # 4 (u)int8 elements in (u)int32
+ assert ic_bn % int8_elems == 0 # (u)int8 elements in (u)int32
Review comment:
@giuseros ,
* In short it mean: "int8 element, booth uint8(weight), uint8(data) products are accumulated up into int32 lanes."
* Comment still valid,```ic_bn``` should be times of ```int8_elems``` (inner loop cannot be tensorized otherwise).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] giuseros commented on a change in pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#discussion_r514421590
##########
File path: python/tvm/topi/generic/conv2d.py
##########
@@ -51,7 +51,7 @@ def fallback_schedule_cpu_common_int8(cfg, wkl, int32_lanes, num_int8_elements):
num_int8_elements,
)
- oc_bn = int32_lanes
+ oc_bn = int32_lanes if int32_lanes >= num_int8_elements else num_int8_elements
Review comment:
Sorry if I don't understand this, but how the number of int32_lanes s can change? Isn't it a function of the data-type and the vector size of the machine?
##########
File path: python/tvm/topi/generic/conv2d.py
##########
@@ -180,7 +180,7 @@ def schedule_conv_NCHWc_cpu_common_int8(
ow_chunk, ow_block = s[CC].split(ow, factor=reg_n)
assert oc_bn % int32_lanes == 0
- assert ic_bn % 4 == 0 # 4 (u)int8 elements in (u)int32
+ assert ic_bn % int8_elems == 0 # (u)int8 elements in (u)int32
Review comment:
Are you sure that this comment is still valid? I think it might be removed, but otherwise I would write: "input channels should be a multiple of the numer of int8 elements we read"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] vinx13 merged pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] cbalint13 commented on a change in pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on a change in pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#discussion_r514604768
##########
File path: python/tvm/topi/generic/conv2d.py
##########
@@ -51,7 +51,7 @@ def fallback_schedule_cpu_common_int8(cfg, wkl, int32_lanes, num_int8_elements):
num_int8_elements,
)
- oc_bn = int32_lanes
+ oc_bn = int32_lanes if int32_lanes >= num_int8_elements else num_int8_elements
Review comment:
@giuseros,
It is "vector size" of a specific machine, but can vary from "machine" to "machine".
* From HW point ```lanes``` are registers (N x 32bit) where data products ```num_int8``` accumulates after multiplications.
* I.E. case of ARM & Intel, the design is: 16 lanes (16 registers) where 4 multiplies (instant ones) accumulates into each.
----
* Now e.g. MARLANN uses 2 lanes, each one accumulating 8 multiplies, see: [ISA](https://github.com/SymbioticEDA/MARLANN/blob/master/docs/isa.md)
* Since ```2 < 8``` (the MARLANN case), introduced ```>=``` to swap final inner&outer tensorisation loop order.
* Working on a more generic HW accelerator where design have parametrizable: ```lanes```, ```macs```.
**Goal:** would like to reuse existing TVM routines as templates, but without the hard coded lanes/macs .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] cbalint13 removed a comment on pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
cbalint13 removed a comment on pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#issuecomment-722273265
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] vinx13 commented on pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#issuecomment-723378800
@cbalint13 could you try restarting the CI?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] cbalint13 commented on pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#issuecomment-722273265
> Thanks for the explanation and for the pointers @cbalint13 ! This LGTM
@giuseros ,
Could please also set "Approve" flag in the review section ?
I am afraid the LGTM sentence is not a sufficient marker.
Thank You !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] cbalint13 commented on pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#issuecomment-722353622
> I didn't know I had the possibility to approve PRs without being a "reviewer". Apparently yes :)
* Uhhm, sorry for that, didn't know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] cbalint13 commented on pull request #6714: More flexible conv2d_NCHWc_int8 generic operator.
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #6714:
URL: https://github.com/apache/incubator-tvm/pull/6714#issuecomment-723428828
> @cbalint13 could you try restarting the CI?
@vinx13 , done, passed.
Thank You !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org