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 2022/03/24 20:28:51 UTC
[GitHub] [tvm] masahi opened a new pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
masahi opened a new pull request #10773:
URL: https://github.com/apache/tvm/pull/10773
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#issuecomment-1080506107
LGTM, I'll wait for @vinx13 to take a look 😸
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r834724420
##########
File path: python/tvm/relay/op/strategy/arm_cpu.py
##########
@@ -88,20 +88,21 @@ def conv2d_strategy_arm_cpu(attrs, inputs, out_type, target):
if groups == 1:
if layout == "NCHW":
if kernel_layout == "OIHW":
- # ARM conv2d spatial pack schedule.
- strategy.add_implementation(
- wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_spatial_pack),
- wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_spatial_pack),
- name="conv2d_nchw_spatial_pack.arm_cpu",
- )
-
if topi.arm_cpu.is_int8_hw_support(data.dtype, kernel.dtype):
strategy.add_implementation(
wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_int8),
wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_int8),
name="conv2d_nchw_int8.arm_cpu",
)
else:
+ # ARM conv2d spatial pack schedule.
+ strategy.add_implementation(
+ wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_spatial_pack),
+ wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_spatial_pack),
+ name="conv2d_nchw_spatial_pack.arm_cpu",
+ plevel=10,
+ )
+
Review comment:
`schedule_conv2d_nchw_spatial_pack` has tophub entries, so they are always picked up based on "time cost" even if `topi.arm_cpu.is_int8_hw_support` path is taken and the NCHWc int8 schedule is surely faster.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r834728177
##########
File path: python/tvm/relay/op/strategy/arm_cpu.py
##########
@@ -88,20 +88,21 @@ def conv2d_strategy_arm_cpu(attrs, inputs, out_type, target):
if groups == 1:
if layout == "NCHW":
if kernel_layout == "OIHW":
- # ARM conv2d spatial pack schedule.
- strategy.add_implementation(
- wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_spatial_pack),
- wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_spatial_pack),
- name="conv2d_nchw_spatial_pack.arm_cpu",
- )
-
if topi.arm_cpu.is_int8_hw_support(data.dtype, kernel.dtype):
strategy.add_implementation(
wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_int8),
wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_int8),
name="conv2d_nchw_int8.arm_cpu",
)
else:
+ # ARM conv2d spatial pack schedule.
+ strategy.add_implementation(
+ wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_spatial_pack),
+ wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_spatial_pack),
+ name="conv2d_nchw_spatial_pack.arm_cpu",
+ plevel=10,
+ )
+
Review comment:
Seems like another good argument for removing tophub. You can disable it for the test by putting it all in an empty `ApplyHistoryBest`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 merged pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #10773:
URL: https://github.com/apache/tvm/pull/10773
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] u99127 commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
u99127 commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r835338505
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -370,29 +352,42 @@ def get_ref_data():
lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
topi.cuda.schedule_conv2d_NCHWc_int8,
4,
+ False,
),
# Disable on CI since it does not support spirv int8 dot product
# (
# "vulkan -from_device=0",
# lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
# topi.cuda.schedule_conv2d_NCHWc_int8,
# 4,
+ # False,
# ),
]
- # TODO(Mousius) Re-enable once implementation is fixed
- # if in_dtype == "int8":
- # targets.append(
- # (
- # "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon",
- # topi.arm_cpu.conv2d_NCHWc_int8,
- # topi.arm_cpu.schedule_conv2d_NCHWc_int8,
- # 8,
- # )
- # )
-
- for target, compute, schedule, oc_block_factor in targets:
- check_target(target, compute, schedule, oc_block_factor)
+ # TODO(tvm-team): Properly run ARM code on CI aarch64 environment
+ targets.append(
+ (
+ "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon,+v8.2a,+dotprod",
Review comment:
Not all AArch64 hardware has the dot product i.e. if someone runs the test suite on a Raspberry pi , this script will fail .
The AWS instance has the dot product instruction, thus perhaps this needs to be guarded with a check for the underlying hardware supports the dot product instruction.
/proc/cpuinfo should contain asimddp for that particular feature .
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#issuecomment-1078740526
@vinx13 would you mind taking a look? Thanks a lot!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r836274968
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -370,29 +352,42 @@ def get_ref_data():
lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
topi.cuda.schedule_conv2d_NCHWc_int8,
4,
+ False,
),
# Disable on CI since it does not support spirv int8 dot product
# (
# "vulkan -from_device=0",
# lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
# topi.cuda.schedule_conv2d_NCHWc_int8,
# 4,
+ # False,
# ),
]
- # TODO(Mousius) Re-enable once implementation is fixed
- # if in_dtype == "int8":
- # targets.append(
- # (
- # "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon",
- # topi.arm_cpu.conv2d_NCHWc_int8,
- # topi.arm_cpu.schedule_conv2d_NCHWc_int8,
- # 8,
- # )
- # )
-
- for target, compute, schedule, oc_block_factor in targets:
- check_target(target, compute, schedule, oc_block_factor)
+ # TODO(tvm-team): Properly run ARM code on CI aarch64 environment
+ targets.append(
+ (
+ "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon,+v8.2a,+dotprod",
Review comment:
Can we enable running this code in the container by using something like:
```
build_only_aarch64 = platform.machine() != "aarch64"
```
That'll mean we'll know if it breaks again 😸
Ultimately, this should be ported to `pytest` parameters and we can add the support checks then?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r836287714
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -370,29 +352,42 @@ def get_ref_data():
lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
topi.cuda.schedule_conv2d_NCHWc_int8,
4,
+ False,
),
# Disable on CI since it does not support spirv int8 dot product
# (
# "vulkan -from_device=0",
# lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
# topi.cuda.schedule_conv2d_NCHWc_int8,
# 4,
+ # False,
# ),
]
- # TODO(Mousius) Re-enable once implementation is fixed
- # if in_dtype == "int8":
- # targets.append(
- # (
- # "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon",
- # topi.arm_cpu.conv2d_NCHWc_int8,
- # topi.arm_cpu.schedule_conv2d_NCHWc_int8,
- # 8,
- # )
- # )
-
- for target, compute, schedule, oc_block_factor in targets:
- check_target(target, compute, schedule, oc_block_factor)
+ # TODO(tvm-team): Properly run ARM code on CI aarch64 environment
+ targets.append(
+ (
+ "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon,+v8.2a,+dotprod",
Review comment:
oh to be clear, this PR doesn't fix the accuracy problem. It only fixes the compiler error when using dot production instructions with the NCHWc layout. The accuracy problem is there even with a default schedule.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r834731807
##########
File path: python/tvm/relay/op/strategy/arm_cpu.py
##########
@@ -88,20 +88,21 @@ def conv2d_strategy_arm_cpu(attrs, inputs, out_type, target):
if groups == 1:
if layout == "NCHW":
if kernel_layout == "OIHW":
- # ARM conv2d spatial pack schedule.
- strategy.add_implementation(
- wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_spatial_pack),
- wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_spatial_pack),
- name="conv2d_nchw_spatial_pack.arm_cpu",
- )
-
if topi.arm_cpu.is_int8_hw_support(data.dtype, kernel.dtype):
strategy.add_implementation(
wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_int8),
wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_int8),
name="conv2d_nchw_int8.arm_cpu",
)
else:
+ # ARM conv2d spatial pack schedule.
+ strategy.add_implementation(
+ wrap_compute_conv2d(topi.arm_cpu.conv2d_nchw_spatial_pack),
+ wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_nchw_spatial_pack),
+ name="conv2d_nchw_spatial_pack.arm_cpu",
+ plevel=10,
+ )
+
Review comment:
The annoying thing is this happens on only specific input or filter sizes. It took me some time to figure out why the NCHWc schedule is not used when the input shape is (56, 56) and filter size is 3x3. If I use (128, 128) shape or 1x1 filter, I didn't see this issue.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r835565747
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -370,29 +352,42 @@ def get_ref_data():
lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
topi.cuda.schedule_conv2d_NCHWc_int8,
4,
+ False,
),
# Disable on CI since it does not support spirv int8 dot product
# (
# "vulkan -from_device=0",
# lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
# topi.cuda.schedule_conv2d_NCHWc_int8,
# 4,
+ # False,
# ),
]
- # TODO(Mousius) Re-enable once implementation is fixed
- # if in_dtype == "int8":
- # targets.append(
- # (
- # "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon",
- # topi.arm_cpu.conv2d_NCHWc_int8,
- # topi.arm_cpu.schedule_conv2d_NCHWc_int8,
- # 8,
- # )
- # )
-
- for target, compute, schedule, oc_block_factor in targets:
- check_target(target, compute, schedule, oc_block_factor)
+ # TODO(tvm-team): Properly run ARM code on CI aarch64 environment
+ targets.append(
+ (
+ "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon,+v8.2a,+dotprod",
Review comment:
I added this test to verify only the build works (`build_only=True`). But yes, if we do want to run the compiled code on CI, we should add such check.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r836305202
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -370,29 +352,42 @@ def get_ref_data():
lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
topi.cuda.schedule_conv2d_NCHWc_int8,
4,
+ False,
),
# Disable on CI since it does not support spirv int8 dot product
# (
# "vulkan -from_device=0",
# lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
# topi.cuda.schedule_conv2d_NCHWc_int8,
# 4,
+ # False,
# ),
]
- # TODO(Mousius) Re-enable once implementation is fixed
- # if in_dtype == "int8":
- # targets.append(
- # (
- # "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon",
- # topi.arm_cpu.conv2d_NCHWc_int8,
- # topi.arm_cpu.schedule_conv2d_NCHWc_int8,
- # 8,
- # )
- # )
-
- for target, compute, schedule, oc_block_factor in targets:
- check_target(target, compute, schedule, oc_block_factor)
+ # TODO(tvm-team): Properly run ARM code on CI aarch64 environment
+ targets.append(
+ (
+ "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon,+v8.2a,+dotprod",
Review comment:
Understood 😸 !
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #10773: [ARM] Fix NCHWc int8 dot product schedule lowering
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #10773:
URL: https://github.com/apache/tvm/pull/10773#discussion_r836284598
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -370,29 +352,42 @@ def get_ref_data():
lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
topi.cuda.schedule_conv2d_NCHWc_int8,
4,
+ False,
),
# Disable on CI since it does not support spirv int8 dot product
# (
# "vulkan -from_device=0",
# lambda a, w, s, p, d, l, ol, o: topi.cuda.conv2d_NCHWc_int8(a, w, s, p, d, l, o),
# topi.cuda.schedule_conv2d_NCHWc_int8,
# 4,
+ # False,
# ),
]
- # TODO(Mousius) Re-enable once implementation is fixed
- # if in_dtype == "int8":
- # targets.append(
- # (
- # "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon",
- # topi.arm_cpu.conv2d_NCHWc_int8,
- # topi.arm_cpu.schedule_conv2d_NCHWc_int8,
- # 8,
- # )
- # )
-
- for target, compute, schedule, oc_block_factor in targets:
- check_target(target, compute, schedule, oc_block_factor)
+ # TODO(tvm-team): Properly run ARM code on CI aarch64 environment
+ targets.append(
+ (
+ "llvm -device arm_cpu -mtriple aarch64-linux-gnu -mattr=+neon,+v8.2a,+dotprod",
Review comment:
Yeah I can do that, but as you already found, ARM conv2d NCHWc op doesn't output correct results at the moment (It looks like the compute itself is busted, so the schedule doesn't matter). I'm going to fix that soon and add such fine-grained check to actually run things.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org