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/03/02 23:27:35 UTC
[GitHub] [tvm] masahi opened a new pull request #7572: [Vulkan] Support passing 64 bit scalar
masahi opened a new pull request #7572:
URL: https://github.com/apache/tvm/pull/7572
----------------------------------------------------------------
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] [tvm] masahi commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790004510
Just found that Metal codegen generates hardcoded `__TVMArgUnion` union:
https://github.com/apache/tvm/blob/053347c784bf891b188a8d7f4d6f4d98eb05deb1/src/target/source/codegen_metal.cc#L49-L51
Do we need to change this to something like this?
```
decl_stream << "union __TVMArgUnion64 {\n"
<< " int32_t v_int32[2];\n"
<< " int64_t v_int64;\n"
<< "};\n\n";
```
And I don't understand why float32 is not there.
----------------------------------------------------------------
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] [tvm] jroesch commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-789335741
Does this work for f64? I think that was the error I triggered the other night
----------------------------------------------------------------
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] [tvm] vinx13 commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587846161
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -47,7 +47,7 @@ CodeGenMetal::CodeGenMetal() {
decl_stream << "#include <metal_stdlib>\n";
decl_stream << "using namespace metal;\n\n";
decl_stream << "union __TVMArgUnion {\n"
- << " int v_int;\n"
+ << " int v_long;\n"
Review comment:
https://github.com/apache/tvm/blob/7d1f47f8ac49718f05981ee89c56f18d7bfb4d01/src/target/source/codegen_metal.cc#L113 Variables in this union is referred by type e.g `v_int, v_char`, but the `ArgUnion` only had `v_int` so I this it won't work.
I can keep `v_int[2]` for now
----------------------------------------------------------------
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] [tvm] tqchen edited a comment on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790183812
we should still keep the 32 bit path, instead generating a int32[2] type as in our argunion
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r586723321
##########
File path: src/runtime/pack_args.h
##########
@@ -40,13 +40,24 @@ namespace tvm {
namespace runtime {
/*!
* \brief argument union type of 32bit.
- * Choose 32 bit because most GPU API do not work well with 64 bit.
*/
union ArgUnion {
Review comment:
done
----------------------------------------------------------------
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] [tvm] tqchen commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587831793
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -104,8 +104,8 @@ void CodeGenMetal::AddFunction(const PrimFunc& f) {
if (v.dtype().bits() == 32) {
decl_stream << " ";
PrintType(v.dtype(), decl_stream);
- decl_stream << " " << vid << ";\n";
- vref << varg << "." << vid;
+ decl_stream << " " << vid << "[2];\n";
+ vref << varg << "." << vid << "[0]";
Review comment:
add also bits() == 64 support, explicitly
----------------------------------------------------------------
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] [tvm] masahi commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-791209821
Ready to merge? @vinx13 @tqchen @jroesch
----------------------------------------------------------------
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] [tvm] tqchen commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r586477695
##########
File path: src/runtime/pack_args.h
##########
@@ -40,13 +40,24 @@ namespace tvm {
namespace runtime {
/*!
* \brief argument union type of 32bit.
- * Choose 32 bit because most GPU API do not work well with 64 bit.
*/
union ArgUnion {
Review comment:
Let us rename to ArgUnion32 to avoid confusion
----------------------------------------------------------------
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] [tvm] vinx13 edited a comment on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 edited a comment on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790851476
I can help with it. @tqchen are you suggesting separating 32bit and 64 bit when passing args?
----------------------------------------------------------------
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] [tvm] tqchen commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790183812
@masahi we should still keep the 32 bit path, instead generating a int32[2] type as in our argunion
----------------------------------------------------------------
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] [tvm] masahi commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-789337533
`ArgUnion` is also used by other runtime (cuda/opencl I think) https://github.com/apache/tvm/blob/1831c17998b29f3797f364410980809bfef554ca/src/runtime/pack_args.h#L143
I was not sure / haven't look if we can replace this with 64.
----------------------------------------------------------------
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] [tvm] vinx13 commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790182582
@tqchen @masahi https://github.com/apache/tvm/blob/6cd9626d620cbd18bbd833ab0c7adbeb7e516409/src/runtime/metal/metal_module.mm#L200 here the `encoder` assumes every argument is 64 bit, I think we need to remove this block https://github.com/apache/tvm/blob/053347c784bf891b188a8d7f4d6f4d98eb05deb1/src/target/source/codegen_metal.cc#L104-L109 and use `TVMArgUnion64` for argument values
----------------------------------------------------------------
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] [tvm] tqchen merged pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #7572:
URL: https://github.com/apache/tvm/pull/7572
----------------------------------------------------------------
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] [tvm] vinx13 commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790069190
@masahi Actually I don't think `int` (and `float32`) should be there, since 32-bit args are captured here https://github.com/apache/tvm/blob/053347c784bf891b188a8d7f4d6f4d98eb05deb1/src/target/source/codegen_metal.cc#L104-L109
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587868073
##########
File path: tests/python/topi/python/test_topi_cumsum.py
##########
@@ -40,10 +42,15 @@ def check_cumsum(np_ref, data, axis=None, dtype=None):
check_cumsum(np.cumsum(data, dtype=np.int32), data)
check_cumsum(np.cumsum(data), data, dtype="int64")
- data = np.random.rand(10) > 0.5
- check_cumsum(np.cumsum(data, dtype=np.int32), data, dtype="int32")
+ if target != "vulkan":
Review comment:
Yes I also found that problem. Try running `pytest -k cumsum`. When run by pytest, target is always a string.
----------------------------------------------------------------
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] [tvm] tqchen commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587854109
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -47,7 +47,7 @@ CodeGenMetal::CodeGenMetal() {
decl_stream << "#include <metal_stdlib>\n";
decl_stream << "using namespace metal;\n\n";
decl_stream << "union __TVMArgUnion {\n"
- << " int v_int;\n"
+ << " int v_long;\n"
Review comment:
sounds good. I think that part also needs some work 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi edited a comment on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-789337533
`ArgUnion` is also used by other runtime (cuda/opencl I think) https://github.com/apache/tvm/blob/1831c17998b29f3797f364410980809bfef554ca/src/runtime/pack_args.h#L143
I was not sure / haven't look if we can replace this with 64. Since `ArgUnion` only supports 32 bit and I haven't seen any issue with it, I'm not sure if replacing this with 64 bit union is a good idea. Thoughts?
----------------------------------------------------------------
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] [tvm] vinx13 commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587865697
##########
File path: tests/python/topi/python/test_topi_cumsum.py
##########
@@ -40,10 +42,15 @@ def check_cumsum(np_ref, data, axis=None, dtype=None):
check_cumsum(np.cumsum(data, dtype=np.int32), data)
check_cumsum(np.cumsum(data), data, dtype="int64")
- data = np.random.rand(10) > 0.5
- check_cumsum(np.cumsum(data, dtype=np.int32), data, dtype="int32")
+ if target != "vulkan":
Review comment:
this won't work because now target has some default attrs e.g. `vulkan -keys=vulkan,gpu -max_num_threads=256`
----------------------------------------------------------------
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] [tvm] masahi commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790708074
I can let @vinx13 directly push to my branch
----------------------------------------------------------------
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] [tvm] tqchen commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790153208
@vinx13 @jwfromm @junrushao1994 would be great if any of you who have a macbook can help double check the metal impl
I think we only need to change https://github.com/apache/tvm/blob/053347c784bf891b188a8d7f4d6f4d98eb05deb1/src/target/source/codegen_metal.cc#L104 to print specific types.
@masahi I think your change to `__TArgUnion` is right. Note that we only use ArgUnion to pass in values that are non-32 bits(e.g. int8 that needs to be passed).
We need to update the generation of the argument buffer struct for 32bit and 64bit values in L104
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r586023246
##########
File path: src/runtime/pack_args.h
##########
@@ -40,13 +40,24 @@ namespace tvm {
namespace runtime {
/*!
* \brief argument union type of 32bit.
- * Choose 32 bit because most GPU API do not work well with 64 bit.
*/
union ArgUnion {
Review comment:
This supports 32 bit only use case. Not sure if we can replace this with 64 bit union.
----------------------------------------------------------------
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] [tvm] tqchen commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587832759
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -47,7 +47,7 @@ CodeGenMetal::CodeGenMetal() {
decl_stream << "#include <metal_stdlib>\n";
decl_stream << "using namespace metal;\n\n";
decl_stream << "union __TVMArgUnion {\n"
- << " int v_int;\n"
+ << " int v_long;\n"
Review comment:
should it be int v_int[2]?
----------------------------------------------------------------
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] [tvm] vinx13 commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790851476
I can help with it. @tqchen are you suggesting separating 32bit and 64 bit args?
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r586022330
##########
File path: src/runtime/pack_args.h
##########
@@ -177,25 +188,28 @@ template <int N, typename F>
inline PackedFunc PackFuncNonBufferArg_(F f, int base, const std::vector<ArgConvertCode>& codes) {
int num_args = static_cast<int>(codes.size());
auto ret = [f, codes, base, num_args](TVMArgs args, TVMRetValue* ret) {
- TempArray<ArgUnion, N> holder_(num_args);
- ArgUnion* holder = holder_.data();
+ TempArray<ArgUnion64, N> holder_(num_args);
+ ArgUnion64* holder = holder_.data();
for (int i = 0; i < num_args; ++i) {
switch (codes[i]) {
- case INT64_TO_INT64:
+ case INT64_TO_INT64: {
+ holder[i].v_int64 = args.values[base + i].v_int64;
+ break;
+ }
case FLOAT64_TO_FLOAT64: {
- LOG(FATAL) << "Do not support 64bit argument to device function";
+ holder[i].v_float64 = args.values[base + i].v_float64;
Review comment:
yes, previously both int64 and fp64 had an issue for the same reason, they are both supported now.
----------------------------------------------------------------
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] [tvm] tqchen edited a comment on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-789766901
cc @vinx13 @zxy844288792 @jwfromm can you please help if the change works for metal GPU backend? 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] [tvm] masahi commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790147387
@tqchen @vinx13 @jroesch @jwfromm I can leave the Metal runtime as an old version and only update vulkan runtime, if that sounds better. Unless someone looks at the Metal issue in detail for me there is nothing I can do.
----------------------------------------------------------------
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] [tvm] tqchen commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790909772
@vinx13 we can follow the same convention, e.g. pass int32 as int32[2] and waste the second element
----------------------------------------------------------------
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] [tvm] tqchen commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587831793
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -104,8 +104,8 @@ void CodeGenMetal::AddFunction(const PrimFunc& f) {
if (v.dtype().bits() == 32) {
decl_stream << " ";
PrintType(v.dtype(), decl_stream);
- decl_stream << " " << vid << ";\n";
- vref << varg << "." << vid;
+ decl_stream << " " << vid << "[2];\n";
+ vref << varg << "." << vid << "[0]";
Review comment:
add also bits() == 64 support
----------------------------------------------------------------
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] [tvm] tqchen commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-791614580
Thanks @masahi @vinx13 @jroesch !
----------------------------------------------------------------
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] [tvm] tqchen commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-789766901
cc @vinx13 @zxy844288792 can you please help if the change works for metal GPU backend? 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] [tvm] vinx13 commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587837241
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -47,7 +47,7 @@ CodeGenMetal::CodeGenMetal() {
decl_stream << "#include <metal_stdlib>\n";
decl_stream << "using namespace metal;\n\n";
decl_stream << "union __TVMArgUnion {\n"
- << " int v_int;\n"
+ << " int v_long;\n"
Review comment:
if I add bits()==64 support explicitly (which doesn't use ArgUnion), is there any case we will use `ArgUnion`? Looks like previous this union hardcoded to only support `int`
----------------------------------------------------------------
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] [tvm] tqchen commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587842759
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -47,7 +47,7 @@ CodeGenMetal::CodeGenMetal() {
decl_stream << "#include <metal_stdlib>\n";
decl_stream << "using namespace metal;\n\n";
decl_stream << "union __TVMArgUnion {\n"
- << " int v_int;\n"
+ << " int v_long;\n"
Review comment:
previously it was intended for passing in lower number of bits (e.g. uint8 and i16). But not sure the driver side support it yet
----------------------------------------------------------------
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] [tvm] jroesch commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r586020194
##########
File path: src/runtime/pack_args.h
##########
@@ -177,25 +188,28 @@ template <int N, typename F>
inline PackedFunc PackFuncNonBufferArg_(F f, int base, const std::vector<ArgConvertCode>& codes) {
int num_args = static_cast<int>(codes.size());
auto ret = [f, codes, base, num_args](TVMArgs args, TVMRetValue* ret) {
- TempArray<ArgUnion, N> holder_(num_args);
- ArgUnion* holder = holder_.data();
+ TempArray<ArgUnion64, N> holder_(num_args);
+ ArgUnion64* holder = holder_.data();
for (int i = 0; i < num_args; ++i) {
switch (codes[i]) {
- case INT64_TO_INT64:
+ case INT64_TO_INT64: {
+ holder[i].v_int64 = args.values[base + i].v_int64;
+ break;
+ }
case FLOAT64_TO_FLOAT64: {
- LOG(FATAL) << "Do not support 64bit argument to device function";
+ holder[i].v_float64 = args.values[base + i].v_float64;
Review comment:
This is the case I was hitting, this answers my question.
----------------------------------------------------------------
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] [tvm] tqchen commented on pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#issuecomment-790703840
@masahi please still incorporate the metal codegen change, otherwise the constant path would results in error(since we already pass in ArgUnion64. Perhaps @vinx13 can followup to suggest a change
----------------------------------------------------------------
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] [tvm] jroesch commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r586020090
##########
File path: src/runtime/pack_args.h
##########
@@ -40,13 +40,24 @@ namespace tvm {
namespace runtime {
/*!
* \brief argument union type of 32bit.
- * Choose 32 bit because most GPU API do not work well with 64 bit.
*/
union ArgUnion {
Review comment:
Is there a reason we duplicated this union?
----------------------------------------------------------------
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] [tvm] vinx13 commented on a change in pull request #7572: [Vulkan] Support passing 64 bit scalar
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #7572:
URL: https://github.com/apache/tvm/pull/7572#discussion_r587846161
##########
File path: src/target/source/codegen_metal.cc
##########
@@ -47,7 +47,7 @@ CodeGenMetal::CodeGenMetal() {
decl_stream << "#include <metal_stdlib>\n";
decl_stream << "using namespace metal;\n\n";
decl_stream << "union __TVMArgUnion {\n"
- << " int v_int;\n"
+ << " int v_long;\n"
Review comment:
https://github.com/apache/tvm/blob/7d1f47f8ac49718f05981ee89c56f18d7bfb4d01/src/target/source/codegen_metal.cc#L113 Variables in this union is referred by type e.g `v_int, v_char`, but the `ArgUnion` only had `v_int` so I think it won't work.
I can keep `v_int[2]` for now
----------------------------------------------------------------
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