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 2021/06/28 16:52:59 UTC
[GitHub] [tvm] AnastasiaStulova opened a new pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
AnastasiaStulova opened a new pull request #8361:
URL: https://github.com/apache/tvm/pull/8361
When dilation is larger than value 1 in conv2d with NHWC
layout, the ordering of indexes when accessing data array
in computation of convolution appears to be incorrect.
'data_vec' is defined as
lambda n, oho, owo, kh, kw, ic, ohi, owi:
but accessed as
data_vec[n, oho, owo, kh, kw, ohi, owi, ic]
This patch fixes the order of indexes.
=============================================
**Question:** While the bug is easily observable I wonder if
there is some way to improve testing of this even if it might
not be possible to add in CI straight away.
I used `tests/python/topi/python/test_topi_conv2d_nhwc.py`
to catch and test the issue. If it makes sense I could prepare
another change adding `arm_cpu` in the list of test devices
and/or even running using RPC that can be activated by
passing a certain flag.
--
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] giuseros commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-870633463
@mbaret @leandron could you double check this and possibly merge?
--
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 pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
u99127 commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-871228909
> This patch looks good to me, ideally we should have test cases for this though. I'm not up to speed on the status of Arm CI so am unsure how immediately feasible this is, perhaps @u99127 or @leandron know more.
do the existing topi tests catch the failure on AArch64 ?
--
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] mbaret commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-870670939
This patch looks good to me, ideally we should have test cases for this though. I'm not up to speed on the status of Arm CI so am unsure how immediately feasible this is, perhaps @u99127 or @leandron know more.
--
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] jcf94 commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
jcf94 commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-876469576
Thanks! @u99127 @AnastasiaStulova
--
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] AnastasiaStulova commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
AnastasiaStulova commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-871364901
> > This patch looks good to me, ideally we should have test cases for this though. I'm not up to speed on the status of Arm CI so am unsure how immediately feasible this is, perhaps @u99127 or @leandron know more.
>
> do the existing topi tests catch the failure on AArch64 ?
@u99127 The test indeed caught the bug and it already what we need for `arm_cpu` testing
https://github.com/apache/tvm/blob/main/tests/python/topi/python/test_topi_conv2d_nhwc.py#L33
but it is not enabled by default in the list of tested devices:
https://github.com/apache/tvm/blob/main/tests/python/topi/python/test_topi_conv2d_nhwc.py#L78
It is very easy to add it to the list `"llvm -device=arm_cpu"` then it runs straight away.
--
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] AnastasiaStulova commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
AnastasiaStulova commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-871367727
FYI there is also [https://github.com/apache/tvm/blob/main/tests/python/topi/python/test_topi_conv2d_nchw.py](https://github.com/apache/tvm/blob/main/tests/python/topi/python/test_topi_conv2d_nchw.py) for testing `NCHW` layout that already runs on all available targets. So if we run it where Arm CPU is enabled it will test it automatically.
The only problem could be that it is a bit long-running as it has more than 350 test cases but we could think of creating a fast running mode too.
--
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] AnastasiaStulova commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
AnastasiaStulova commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-869845657
@giuseros
--
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 pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
u99127 commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-872585551
> FYI there is also https://github.com/apache/tvm/blob/main/tests/python/topi/python/test_topi_conv2d_nchw.py for testing `NCHW` layout that already runs on all available targets. So if we run it where Arm CPU is enabled it will test it automatically.
>
> The only problem could be that it is a bit long-running as it has more than 350 test cases but we could think of creating a fast running mode too.
Got it - yes I could just reproduce the failure with that test but as you say I did have to add 'llvm -device=arm_cpu' to the devices list as you said to see the AssertionError failure, so may be the solution here is that we add 'llvm -device=arm_cpu' to the test here or actually fix the test to only use 'llvm -device=arm_cpu' when running on ci_arm rather than having it run twice on the GPU CI which is where TOPI testing is turned on.
I suspect the following things need to happen here to progress this with the tests.
1. We run the topi tests in Jenkins using the Task script and just turn that on for the CI on AArch64. That is a one liner change and hopefully it works. However while pipe cleaning that on my local machine , I've hit a few testisms in the topi tests that need to be cleaned up. Especially with some tests being suitable only for CUDA but not being marked as so. I can help with this. That's pretty easy and straightforward but will need buy in from the project that running these tests in AArch64 CI is a good thing ! I think this bug just shows the value of running these tests in CI. I would prefer to start with the entirety of the TOPI tests, it appears to take me about 30-40 minutes on a machine that has similar characteristics to the one in CI.
2. I agree with you that we should look to add to this PR an additional change to the test script you mention by adding 'llvm -device=arm_cpu' to the list of targets only if one is running the test on AArch64 hardware, otherwise when we come around to doing 1 above we'd be running this test once for the llvm target and once for an llvm -device=arm_cpu and just duplicating testing for a path that is unneeded . For bonus points we should audit the topi test directory and clean it up as we go along because I note that many tests again run for both llvm and llvm -device=arm_cpu which probably just uses more runtime during CI when it is not needed.
regards
Ramana
--
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] jcf94 merged pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
jcf94 merged pull request #8361:
URL: https://github.com/apache/tvm/pull/8361
--
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] jcf94 edited a comment on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
jcf94 edited a comment on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-876469576
Thanks! @u99127 @AnastasiaStulova @giuseros
--
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 pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
u99127 commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-876468477
I think this can be merged before #8409 as it would make my life easier to know that upstream tests are as clan as possible.
--
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] AnastasiaStulova commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
AnastasiaStulova commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-874725516
This PR now allows enabling testing of this functionality in CI for Aarch64 after the following https://github.com/apache/tvm/pull/8409 gets merged.
--
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] AnastasiaStulova commented on pull request #8361: [Relay] Fix index order in conv2d computation for Arm CPU.
Posted by GitBox <gi...@apache.org>.
AnastasiaStulova commented on pull request #8361:
URL: https://github.com/apache/tvm/pull/8361#issuecomment-869845657
@giuseros
--
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