You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "masahi (via GitHub)" <gi...@apache.org> on 2023/07/30 03:37:51 UTC

[GitHub] [tvm] masahi opened a new pull request, #15439: [Vulkan] Add spirv shuffle instruction support

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

   This allows using `builtin::tvm_warp_shuffle()`, `builtin::tvm_warp_shuffle_up/down()` for VK. We need to specify the warp size explicitly as `target = "vulkan -from_device=0 -thread_warp_size=32` to enable shuffle instructions.
   
   On NV it works correctly. TODO: test on AMD and add unit tests.
   
   On Intel the result is incorrect for some reason. In general subgroup on Intel GPU seems tricky, since unlike NV or AMD there is no well defined "subgroup (warp) size". Each instruction can execute with different subgroup sizes.  To disable shuffle instructions, we can add `-supported_subgroup_operations=0` to the target string. 
   
   Reference:
   https://www.khronos.org/blog/vulkan-subgroup-tutorial
   https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_non_uniform_instructions
   https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpGroupNonUniformShuffleDown


-- 
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] tqchen commented on pull request #15439: [Vulkan] Add spirv shuffle instruction support

Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #15439:
URL: https://github.com/apache/tvm/pull/15439#issuecomment-1659399479

   @mei-ye do you have some insights into this?


-- 
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] masahi commented on a diff in pull request #15439: [Vulkan] Add spirv shuffle instruction support

Posted by "masahi (via GitHub)" <gi...@apache.org>.
masahi commented on code in PR #15439:
URL: https://github.com/apache/tvm/pull/15439#discussion_r1279109603


##########
src/tir/transforms/lower_thread_allreduce.cc:
##########
@@ -719,12 +719,16 @@ class ThreadAllreduceBuilder final : public StmtExprMutator {
   // Also, the warp/wavefront size differs (64 on rocm, 32 on cuda and metal).
   bool IsWarpReduction(const std::vector<DataType>& types, int group_extent, int reduce_extent,
                        int contiguous_reduce_extent) {
-    if ((target_->kind->name != "cuda") && (target_->kind->name != "rocm") &&
-        (target_->kind->name != "metal")) {
+    if (target_->kind->name == "vulkan") {
+      if (target_->GetAttr<Integer>("supported_subgroup_operations") == 0) {

Review Comment:
   Here, ideally we should check the availability of each subgroup feature by bit mask such as `VK_SUBGROUP_FEATURE_SHUFFLE_BIT`, and `VK_SUBGROUP_FEATURE_SHUFFLE_RELATIVE_BIT`. But we cannot include a vulkan header in this file, so for simplicity I'm assuming that non-zero `supported_subgroup_operations` implies that most common shuffle operations are supported. 
   
   Otherwise, we need to have more fine-grained target properties than the bit mask. 



-- 
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] tvm-bot commented on pull request #15439: [Vulkan] Add spirv shuffle instruction support

Posted by "tvm-bot (via GitHub)" <gi...@apache.org>.
tvm-bot commented on PR #15439:
URL: https://github.com/apache/tvm/pull/15439#issuecomment-1657017625

   <!---bot-comment-->
   
   Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
   
   
   
   <sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>


-- 
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] masahi commented on pull request #15439: [Vulkan] Add spirv shuffle instruction support

Posted by "masahi (via GitHub)" <gi...@apache.org>.
masahi commented on PR #15439:
URL: https://github.com/apache/tvm/pull/15439#issuecomment-1657391709

   See https://en.wikipedia.org/wiki/RDNA_(microarchitecture)#Architecture. The HW-native wavefront size of recent consumer GPUs is actually 32. But they can also work with wave64 by breaking up a wave64 instruction into two.


-- 
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] tqchen commented on pull request #15439: [Vulkan] Add spirv shuffle instruction support

Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #15439:
URL: https://github.com/apache/tvm/pull/15439#issuecomment-1657351910

   @masahi reading a bit, https://en.wikipedia.org/wiki/Graphics_Core_Next seems the "wavefront" of AMD is 64 threads, does that mean we should instead use warp_size=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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

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


[GitHub] [tvm] masahi commented on a diff in pull request #15439: [Vulkan] Add spirv shuffle instruction support

Posted by "masahi (via GitHub)" <gi...@apache.org>.
masahi commented on code in PR #15439:
URL: https://github.com/apache/tvm/pull/15439#discussion_r1279109603


##########
src/tir/transforms/lower_thread_allreduce.cc:
##########
@@ -719,12 +719,16 @@ class ThreadAllreduceBuilder final : public StmtExprMutator {
   // Also, the warp/wavefront size differs (64 on rocm, 32 on cuda and metal).
   bool IsWarpReduction(const std::vector<DataType>& types, int group_extent, int reduce_extent,
                        int contiguous_reduce_extent) {
-    if ((target_->kind->name != "cuda") && (target_->kind->name != "rocm") &&
-        (target_->kind->name != "metal")) {
+    if (target_->kind->name == "vulkan") {
+      if (target_->GetAttr<Integer>("supported_subgroup_operations") == 0) {

Review Comment:
   Here, ideally we should check the availability of each subgroup feature by bit mask such as `VK_SUBGROUP_FEATURE_SHUFFLE_BIT`, and `VK_SUBGROUP_FEATURE_SHUFFLE_RELATIVE_BIT`. But we cannot include a vulkan header in this file, so for simplicity I'm assuming that non-zero `supported_subgroup_operations` implies most common shuffle operations are supported. 



-- 
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