[Relay] Fix index order in conv2d computation for Arm CPU. #8361
Conversation
There was a problem hiding this comment.
Nice catch @AnastasiaStulova ! So yes, I agree on the testing part. If you could add a separate PR that enables arm_cpu tests, that would be very helpful
@u99127 The test indeed caught the bug and it already what we need for |
|
FYI there is also https://github.com/apache/tvm/blob/main/tests/python/topi/python/test_topi_conv2d_nchw.py for testing 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.
regards |
AnastasiaStulova
force-pushed
the
armcpu_conv2d_nhwc
branch
from
b5e3063
to
dc557ae
Compare
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 and modifies the test so that it is suitable for running on an AArch64 CPU.
This pull request cleans up the TOPI testuite for use on the AArch64 CI target by doing the following: - Introducing a script to run the tests on AArch64 with a suitable invocation of the llvm target string by setting the TVM_TEST_TARGETS environment variable. - Cleaning up the use of hard coded targets and moving the testsuite to testing more sensibly with the use of tvm.testing.enabled_targets. - Cleanup the use of tvm.target.create. - The above allows for the use of tests reasonably with the topi tests and cleans up what is needed from the testsuite. Putting this up for a test run on ci_gpu and ci_cpu to see the effects of moving TOPI test runs to AArch64 CPU before firing up the Jenkins changes. The motivation was from apache#8361 to pipeclean and add this support.
This pull request cleans up the TOPI testuite for use on the AArch64 CI target by doing the following: - Introducing a script to run the tests on AArch64 with a suitable invocation of the llvm target string by setting the TVM_TEST_TARGETS environment variable. - Cleaning up the use of hard coded targets and moving the testsuite to testing more sensibly with the use of tvm.testing.enabled_targets. - Cleanup the use of tvm.target.create vs tvm.target.Target - The above allows for the use of tests reasonably with the topi tests and cleans up what is needed from the testsuite. Putting this up for a test run on ci_gpu and ci_cpu to see the effects of moving TOPI test runs to AArch64 CPU before firing up the Jenkins changes. The motivation was from apache#8361 to pipeclean and add this support.
|
This PR now allows enabling testing of this functionality in CI for Aarch64 after the following #8409 gets merged. |
AnastasiaStulova
force-pushed
the
armcpu_conv2d_nhwc
branch
from
dc557ae
to
125cb5e
Compare
There was a problem hiding this comment.
Thanks! LGTM.
Does this PR has any dependency with #8409 ? I think it's ready for merge? @AnastasiaStulova
|
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. |
|
Thanks! @u99127 @AnastasiaStulova @giuseros |
This pull request cleans up the TOPI testuite for use on the AArch64 CI target by doing the following: - Introducing a script to run the tests on AArch64 with a suitable invocation of the llvm target string by setting the TVM_TEST_TARGETS environment variable. - Cleaning up the use of hard coded targets and moving the testsuite to testing more sensibly with the use of tvm.testing.enabled_targets. - Cleanup the use of tvm.target.create. - The above allows for the use of tests reasonably with the topi tests and cleans up what is needed from the testsuite. Putting this up for a test run on ci_gpu and ci_cpu to see the effects of moving TOPI test runs to AArch64 CPU before firing up the Jenkins changes. The motivation was from apache#8361 to pipeclean and add this support.
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 and modifies the test so that it is suitable for running on an AArch64 CPU.
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 and modifies the test so that it is suitable for running on an AArch64 CPU.
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 and modifies the test
so that it is suitable for running on an AArch64 CPU.