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/12/08 13:37:30 UTC

[GitHub] [tvm] masahi opened a new pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

masahi opened a new pull request #7056:
URL: https://github.com/apache/tvm/pull/7056


   


----------------------------------------------------------------
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 #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

masahi commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741363193


   Thanks @mbrookhart @tkonolige @zhiics @Laurawly 


----------------------------------------------------------------
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] comaniac edited a comment on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

comaniac edited a comment on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741455504


   No worries. The above solution worked for me. Simply changing to `set(CMAKE_CUDA_FLAGS "${CMAKE_CUDA_FLAGS} --expt-extended-lambda")` in CUDA.cmake.
   


----------------------------------------------------------------
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] comaniac commented on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

comaniac commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741444918


   @masahi my build is broken due to this PR:
   
   ```
   nvcc fatal   : Unknown option '-extended-lambda'
   CMakeFiles/tvm_objs.dir/build.make:4859: recipe for target 'CMakeFiles/tvm_objs.dir/src/runtime/contrib/thrust/thrust.cu.o' failed
   make[2]: *** [CMakeFiles/tvm_objs.dir/src/runtime/contrib/thrust/thrust.cu.o] Error 1
   make[2]: *** Waiting for unfinished jobs....
   CMakeFiles/Makefile2:72: recipe for target 'CMakeFiles/tvm_objs.dir/all' failed
   make[1]: *** [CMakeFiles/tvm_objs.dir/all] Error 2
   Makefile:129: recipe for target 'all' failed
   ```
   
   Is there any new version requirement for nvcc?


----------------------------------------------------------------
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] zhiics commented on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

zhiics commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-740769920


   Yeah, I think we need to fix sort otherwise thrust would be the better way for us to go. There are multiple dependency on it already.


----------------------------------------------------------------
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] mbrookhart commented on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

mbrookhart commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741006719


   I think I agree with you, @masahi, my only disagreement is the level at which we are currently implementing things. In general we should always have a generic implementation with reasonable performance, even if it's not great performance. Say, for instance, we have a stable implementation of sort. Then, when we find a faster kernel for sort via thrust, we specialize the topi sort implementaiton to return thrust instead of tir for that usecase.
   
   At this point, we only need one implementation of scatter, because it just calls into topi sort, and topi does the specialization, instead of having to specialize scatter, topk, nms, etc all for vendor-specific versions of sort.


----------------------------------------------------------------
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] comaniac edited a comment on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

comaniac edited a comment on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741449236


   I found this issue saying that we should use `--expt-extended-lambda` to be compatible to CUDA <10.1
   https://github.com/NVIDIA/MinkowskiEngine/issues/207


----------------------------------------------------------------
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 #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

masahi commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741451229


   oh sorry about that. I will send a fix ASAP


----------------------------------------------------------------
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 #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

masahi commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741047455


   I see, you are right, the current way of directly calling thrust sort from higher level op like scatter is not ideal. Dispatching decisions should be left to `topi.sort`, yes.
   
   One tricky bit is, what I need here is not exactly `topi.sort`, but more specific one like `topi.stable_sort_by_key`. Similar to this case, I imagine each higher level op could end up utilizing sorting in a customized way, so one `topi.sort` may not suffice.
   
    Being able to introduce and use customized sorting op by directly dropping into low level libs is certainly convenient, as demonstrated in this PR, although I have to admit it is a bit ad hoc.   


----------------------------------------------------------------
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] tkonolige commented on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

tkonolige commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-740796393


   Looks great! Just to clarify, this can handle repeated indices right?


----------------------------------------------------------------
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] mbrookhart commented on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

mbrookhart commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-740770869


   I'm working on it :) I'll let you know what I come up with


----------------------------------------------------------------
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 #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

masahi commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-740883003


   > Looks great! Just to clarify, this can handle repeated indices right?
   
   @tkonolige Yes definitely (that's the whole point!!). Repeated indices are grouped together by sorting, and only the last one can scatter its update to the output. The result should always be identical with the current scatter 1D and our numpy reference used in the tests. 


----------------------------------------------------------------
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 #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

masahi commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-740919507

> As a larger conversation, I don't love how much we're depending on thrust for functionality, I'd kind of like to fix the issues in topi around sort so we don't have to lean on thrust so much. We're depending on the nomially cuda topi kernels for a lot of other GPUs, so this trend makes it harder to support more diverse hardware.

@mbrookhart @zhiics While I fully agree with this generally, for fundamental, low level GPU primitives such as sorting, scan etc, I think it would be really hard for generic implementations to match or outperform platform specific libraries. These libraries have years of development behind it and use platform specific intrinsics to maximize performance. This also applies to cuDNN, but unlike convolution op, I don't think AutoTVM or Ansor would help generate efficient sort or scan ops.

Sooner or later, I think we will introduce `cumsum` op to TVM. On CUDA, `cumsum` can be implemented very efficiently via thrust or cub's `inclusive_scan`. But without it, we have to roll our own GPU scan implementation that is supposed to work on all GPU targets, which I think would be a formidable or near impossible task.

So my opinion is, while native TVM solution is always what we should strive for, if there is a platform specific library, we should embrace it. Sort, scan etc are standard enough that there is a good chance platform specific library is available. For example, rocm has their implementation of thrust, on OpenCL there is Boost.compute.

----------------------------------------------------------------
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 merged pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

masahi merged pull request #7056:
URL: https://github.com/apache/tvm/pull/7056


   


----------------------------------------------------------------
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] comaniac commented on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

comaniac commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741449236


   I found this issue saying that we should use `-expt-extended-lambda` to be compatible to CUDA <10.1
   https://github.com/NVIDIA/MinkowskiEngine/issues/207


----------------------------------------------------------------
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] comaniac commented on pull request #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

comaniac commented on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-741455504


   No worries. The above issue worked for me. Simply changing to `set(CMAKE_CUDA_FLAGS "${CMAKE_CUDA_FLAGS} --expt-extended-lambda")` in CUDA.cmake.
   


----------------------------------------------------------------
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 #7056: [TOPI] GPU scatter 1D via sorting based approach

Posted by GitBox <gi...@apache.org>.

masahi edited a comment on pull request #7056:
URL: https://github.com/apache/tvm/pull/7056#issuecomment-740919507

Sooner or later, I think we will introduce `cumsum` op to TVM. On CUDA, `cumsum` can be implemented very efficiently via thrust or cub's `inclusive_scan`. But without it, we have to roll our own GPU scan implementation that can compete with vender-provided one, which I think would be a formidable or near impossible task.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org