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/06/19 23:24:50 UTC
[GitHub] [incubator-tvm] trevor-m opened a new pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
trevor-m opened a new pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857
Some fixes a few months ago to the `get_valid_counts` CUDA implementation broke OpenCL at the same time, because of the atomic add intrinsic which was added.
This PR fixes `get_valid_counts` for OpenCL with the following changes:
1. Register intrinsic atomic add for OpenCL.
2. Override `intrinsic::tvm_address_of` to include storage scope.
3. Enable `cl_khr_global_int32_base_atomics`. This isn't required for [OpenCL 1.1+ because atomic_add became a core feature](https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_int32_atomics). I'm happy to remove this if we don't care about OpenCL 1.0. Alternatively we can override `op->call_type == CallNode::PureExtern` and set a flag to enable this only when `atomic_add` is actually used.
Original error messages before this fix:
1. During compilation: `Unresolved intrinsic atomic_add with return type int32`
2. During runtime:
```
<source>:6922:43: error: casting '__global void *' to type 'int *' changes address space of pointer
atomic_add_return[(0)] = atomic_add(((int *)get_valid_counts_v0 + 0), 1);
```
----------------------------------------------------------------
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] wpan11nv commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
wpan11nv commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r443789609
##########
File path: src/target/source/codegen_opencl.cc
##########
@@ -70,6 +70,10 @@ std::string CodeGenOpenCL::Finish() {
"#endif\n\n";
}
+ // Enable atomic_add used by get_valid_counts. Only needed for OpenCL < 1.1.
Review comment:
use a flag guard this emission? Like fp16 extensions.
----------------------------------------------------------------
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] wpan11nv commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
wpan11nv commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r443789609
##########
File path: src/target/source/codegen_opencl.cc
##########
@@ -70,6 +70,10 @@ std::string CodeGenOpenCL::Finish() {
"#endif\n\n";
}
+ // Enable atomic_add used by get_valid_counts. Only needed for OpenCL < 1.1.
Review comment:
use a flag to guard this emission? Like fp16 extensions.
----------------------------------------------------------------
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] trevor-m commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r445798573
##########
File path: tests/python/relay/test_op_level5.py
##########
@@ -270,8 +270,8 @@ def verify_get_valid_counts(dshape, score_threshold, id_index, score_index):
intrp = relay.create_executor("debug", ctx=ctx, target=target)
out = intrp.evaluate(func)(np_data)
tvm.testing.assert_allclose(out[0].asnumpy(), np_out1, rtol=1e-3, atol=1e-04)
- # get_valid_count for cuda doesn't do data rearrangement
- if target == 'cuda':
+ # get_valid_count for cuda, opencl doesn't do data rearrangement
+ if target in ['cuda', 'opencl']:
return
Review comment:
OpenCL uses the same implementation as CUDA. The CUDA implementation of `get_valid_counts` was changed to no longer rearrange the output of `get_valid_counts` because it will be rearranged by NMS later anyway. This gives the correct output for NMS.
That issue with NMS looks to be a separate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] trevor-m commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r443817236
##########
File path: src/target/source/codegen_opencl.cc
##########
@@ -70,6 +70,10 @@ std::string CodeGenOpenCL::Finish() {
"#endif\n\n";
}
+ // Enable atomic_add used by get_valid_counts. Only needed for OpenCL < 1.1.
Review comment:
Thanks @wpan11nv for reviewing! I touched on this in the description:
> Enable cl_khr_global_int32_base_atomics. This isn't required for OpenCL 1.1+ because atomic_add became a core feature. I'm happy to remove this if we don't care about OpenCL 1.0. Alternatively we can override op->call_type == CallNode::PureExtern and set a flag to enable this only when atomic_add is actually used.
I can definitely add a flag but it would require us to override `VisitExpr()` for `CallNode::PureExtern` just to check if atomic_add is used. Do you think I should do that?
----------------------------------------------------------------
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] trevor-m commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-647770558
> Any unit test?
`RELAY_TEST_TARGETS=opencl python3 tests/python/relay/test_op_level5.py` will test this.
We would need to add `opencl` to `ctx_list` to have this run by default https://github.com/apache/incubator-tvm/blob/master/python/tvm/relay/testing/config.py#L28
Currently the CI doesn't test anything for opencl which is why we don't find out about these errors until much later. Do we know why we don't test opencl?
----------------------------------------------------------------
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] trevor-m commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-648450759
@ wpan11nv Any more comments?
----------------------------------------------------------------
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] trevor-m commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-651368469
@kazum @tqchen Rebased and CI passed. Thanks!
----------------------------------------------------------------
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] kazum commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
kazum commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r445411686
##########
File path: tests/python/relay/test_op_level5.py
##########
@@ -270,8 +270,8 @@ def verify_get_valid_counts(dshape, score_threshold, id_index, score_index):
intrp = relay.create_executor("debug", ctx=ctx, target=target)
out = intrp.evaluate(func)(np_data)
tvm.testing.assert_allclose(out[0].asnumpy(), np_out1, rtol=1e-3, atol=1e-04)
- # get_valid_count for cuda doesn't do data rearrangement
- if target == 'cuda':
+ # get_valid_count for cuda, opencl doesn't do data rearrangement
+ if target in ['cuda', 'opencl']:
return
Review comment:
Returning here looks wrong to me. The test in the below link doesn't work for OpenCL too because we don't do data rearrangement for GPU nms implementation.
https://discuss.tvm.ai/t/nms-compile-fails-for-cuda-target-but-works-fine-for-llvm-target/7045/2
Probably, we should fix non_max_suppression for GPU first?
----------------------------------------------------------------
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] trevor-m edited a comment on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m edited a comment on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-648450759
@wpan11nv Any more comments?
----------------------------------------------------------------
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] trevor-m commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-650261839
> Looks good to me. I'll merge this after CI is passed.
Thanks!
----------------------------------------------------------------
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] trevor-m commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r445798573
##########
File path: tests/python/relay/test_op_level5.py
##########
@@ -270,8 +270,8 @@ def verify_get_valid_counts(dshape, score_threshold, id_index, score_index):
intrp = relay.create_executor("debug", ctx=ctx, target=target)
out = intrp.evaluate(func)(np_data)
tvm.testing.assert_allclose(out[0].asnumpy(), np_out1, rtol=1e-3, atol=1e-04)
- # get_valid_count for cuda doesn't do data rearrangement
- if target == 'cuda':
+ # get_valid_count for cuda, opencl doesn't do data rearrangement
+ if target in ['cuda', 'opencl']:
return
Review comment:
OpenCL uses the same implementation as CUDA. The CUDA implementation of `get_valid_counts` was changed to no longer rearrange the output of `get_valid_counts` because it will be rearranged by NMS later anyway. This gives the correct output for NMS. See https://github.com/apache/incubator-tvm/pull/5339
That issue with NMS looks to be a separate issue where the CUDA implementation wasn't fully updated to match changes to CPU implementation by https://github.com/apache/incubator-tvm/pull/4312/
----------------------------------------------------------------
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] wpan11nv commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
wpan11nv commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r443790680
##########
File path: src/target/source/codegen_opencl.cc
##########
@@ -224,6 +228,25 @@ std::string CodeGenOpenCL::CastFromTo(std::string value, DataType from, DataType
return os.str();
}
+void CodeGenOpenCL::VisitExpr_(const CallNode* op, std::ostream& os) {
+ if (op->is_intrinsic(intrinsic::tvm_address_of)) {
+ // Overload tvm_address_of to add storage scope.
+ const LoadNode* l = op->args[0].as<LoadNode>();
Review comment:
please use a different name. load or ld?
----------------------------------------------------------------
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] kazum commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
kazum commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-651451372
Thanks @trevor-m @wpan11nv !
----------------------------------------------------------------
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] zhiics commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-648929669
@kazum can you take a look and manage the PR? Thanks.
----------------------------------------------------------------
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] trevor-m commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-647636426
@Laurawly @kazum @wpan11nv Could you please review? Thanks!
----------------------------------------------------------------
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] trevor-m commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r445799816
##########
File path: tests/python/relay/test_op_level5.py
##########
@@ -270,8 +270,8 @@ def verify_get_valid_counts(dshape, score_threshold, id_index, score_index):
intrp = relay.create_executor("debug", ctx=ctx, target=target)
out = intrp.evaluate(func)(np_data)
tvm.testing.assert_allclose(out[0].asnumpy(), np_out1, rtol=1e-3, atol=1e-04)
- # get_valid_count for cuda doesn't do data rearrangement
- if target == 'cuda':
+ # get_valid_count for cuda, opencl doesn't do data rearrangement
+ if target in ['cuda', 'opencl']:
return
Review comment:
See https://github.com/apache/incubator-tvm/pull/5339
----------------------------------------------------------------
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] tqchen commented on pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#issuecomment-650835082
@trevor-m please rebase against the master
----------------------------------------------------------------
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] kazum merged pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
kazum merged pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857
----------------------------------------------------------------
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] trevor-m commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r445799816
##########
File path: tests/python/relay/test_op_level5.py
##########
@@ -270,8 +270,8 @@ def verify_get_valid_counts(dshape, score_threshold, id_index, score_index):
intrp = relay.create_executor("debug", ctx=ctx, target=target)
out = intrp.evaluate(func)(np_data)
tvm.testing.assert_allclose(out[0].asnumpy(), np_out1, rtol=1e-3, atol=1e-04)
- # get_valid_count for cuda doesn't do data rearrangement
- if target == 'cuda':
+ # get_valid_count for cuda, opencl doesn't do data rearrangement
+ if target in ['cuda', 'opencl']:
return
Review comment:
See https://github.com/apache/incubator-tvm/pull/5339
----------------------------------------------------------------
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] kazum commented on a change in pull request #5857: [OpenCL] Fix OpenCL get_valid_counts errors due to intrinsic atomic_add
Posted by GitBox <gi...@apache.org>.
kazum commented on a change in pull request #5857:
URL: https://github.com/apache/incubator-tvm/pull/5857#discussion_r446002698
##########
File path: tests/python/relay/test_op_level5.py
##########
@@ -270,8 +270,8 @@ def verify_get_valid_counts(dshape, score_threshold, id_index, score_index):
intrp = relay.create_executor("debug", ctx=ctx, target=target)
out = intrp.evaluate(func)(np_data)
tvm.testing.assert_allclose(out[0].asnumpy(), np_out1, rtol=1e-3, atol=1e-04)
- # get_valid_count for cuda doesn't do data rearrangement
- if target == 'cuda':
+ # get_valid_count for cuda, opencl doesn't do data rearrangement
+ if target in ['cuda', 'opencl']:
return
Review comment:
Thanks for your explanation. Actually, I've successfully build NMS if I revert the change in #4312.
----------------------------------------------------------------
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