Skip to content

Conversation

@masahi
Copy link
Member

@masahi masahi commented May 21, 2021

I've made this change too many times during my development, to workaround topi scatter op that requires thread_warp_size for no good reason. Since #8030 transpose op also requires thread_warp_size and all models that use transpose op are broken on vk.

@tmoreau89 @Lunderberg @junrushao1994 @zxybazh

@zxybazh
Copy link
Member

zxybazh commented May 21, 2021

The code looks good to me. May I know why the default thread_warp_size is 1 here?

@masahi
Copy link
Member Author

masahi commented May 21, 2021

Because it need to support ALL vulkan or opencl devices. 1 is the only value that could work, but it is useless in terms of performance. It's only there so that compiling models on vulkan wouldn't break.

@zxybazh
Copy link
Member

zxybazh commented May 21, 2021

Thanks for the quick reply. In that case, thread_warp_size of 1 should be a good default value.

@Lunderberg
Copy link
Contributor

Looks good to me, and I agree on using 1 as the default thread_warp_size. Both the VkPhysicalDeviceSubgroupProperties and the VkPhysicalDeviceSubgroupSizeControlPropertiesEXT documentation give a minimum subgroup size of 1, so the spec doesn't give us any further guarantees.

@tqchen tqchen merged commit 8934d6b into apache:main May 22, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 17, 2021
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants