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/05/25 17:56:12 UTC
[GitHub] [tvm] Lunderberg opened a new pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Lunderberg opened a new pull request #8127:
URL: https://github.com/apache/tvm/pull/8127
The end-to-end goal of this PR is to be able to specify all vulkan capabilities in the Target, initialize vulkan such that we can legally use those capabilities, then pass the device description to the SPIRVSupport struct. This is broken down into the following subchanges, each of which is implemented in a separate commit for readability.
- Enable instance/device extensions. Vulkan requires that extensions be explicitly enabled if used. Has explicitly list out which extensions are required (currently none) and which are optional.
- Extract device information using `vkGetPhysicalDeviceProperties` and `vkGetPhysicalDeviceFeatures`, determine which Vulkan capabilities are supported, pack into a Target.
- Query instance-supported apiVersion before creating instance. Vulkan requires no functions beyond the specified apiVersion be used.
- Query support for dedicated allocation and push descriptors along with the rest of the device support. Move the options to disable their use from compile-time variables to environment variables `TVM_VULKAN_DISABLE_PUSH_DESCRIPTOR` and `TVM_VULKAN_DISABLE_DEDICATED_ALLOCATION`.
- Move option for vulkan validation layers to environment variable, to enable faster use as a debug tool. If `TVM_VULKAN_ENABLE_VALIDATION_LAYERS` is a non-empty string, validation layers will be enabled.
- Explicitly enable vulkan features in device creation. Vulkan requires that features be explicitly enabled before use. For each feature that the device supports and a shader might use, declare it in the call to `vkCreateDevice`.
- Avoid repeated queries for device attributes, implementing `VulkanDeviceAPI::GetAttr` based on the per-device values stored in the Target. This pulls all logic for querying device parameters is in a single location.
- Implement "from_device" flag for the vulkan target. With the number of device capabilities that may or may not be supported by a vulkan driver, it can be tedious to input them. Specifying "-from_device=0" now indicates that any unspecified values should be read from the device.
- Read vulkan device capabilities/limits from Target. Previously, the codegen assumed that all device features were present. Now, the codegen reads device capabilities from the Target, and throws an error if codegen would require use of an unsupported feature.
--
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 #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848391309
> Though I think it would be best for me to do so in a separate PR, in order to separate out functionality changes from refactoring
Yes that's also what I was thinking, we can do that in a separate PR later.
--
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 #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848335189
I think `vulkan.cc` is getting too big at this point, how about splitting `VulkanModuleNode` and `VulkanDeviceAPI` etc into their own files?
--
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] Lunderberg edited a comment on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
Lunderberg edited a comment on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848383793
Definitely agreed, `vulkan.cc` could do with a bit of splitting for readability at this point. I was also thinking that the `VulkanContext` would be good to split out into a separate file, and could have the device-specific initialization logic moved to it. This PR pushes the `VulkanDeviceAPI` constructor to nearly 300 lines, which is a bit difficult for me to follow.
Edit: Though I think it would be best for me to do so in a separate PR, in order to separate out functionality changes from refactoring.
--
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] Lunderberg commented on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848090555
Potential reviewers: @masahi @zxybazh @tmoreau89
--
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 #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #8127:
URL: https://github.com/apache/tvm/pull/8127
--
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] Lunderberg commented on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848383793
Definitely agreed, `vulkan.cc` could do with a bit of splitting for readability at this point. I was also thinking that the `VulkanContext` would be good to split out into a separate file, and could have the device-specific initialization logic moved to it. This PR pushes the `VulkanDeviceAPI` constructor to nearly 300 lines, which is a bit difficult for me to follow.
--
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 #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848659951
@Lunderberg Very nice! As a follow up, I think we should
* Add warp sync to fix reduction, use subgroup barrier if vulkan 1.1 is available (and update the validator)
* Use a target specific max push constant bytes instead of hard coded value
--
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] Lunderberg commented on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848902009
> @Lunderberg Very nice! As a follow up, I think we should
>
> * Add warp sync to fix reduction, use subgroup barrier if vulkan 1.1 is available (and update the validator)
>
> * Use a target specific max push constant bytes instead of hard coded value
Thank you, and agreed! I've been making mental notes on other items that we can check or conditionally include, which will probably go in after the refactoring.
* Pass boolean arrays in using smallest available integer type, instead of only 8-bit int.
* Verify maximum UBO size (unlikely that we'll even get close to the spec-guaranteed size of 16k, but worth checking)
* For loop indices, use largest available integer type, instead of only 64-bit 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] masahi commented on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848202540
please fix the lint error
--
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 #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848335189
I think `vulkan.cc` is getting too big at this point, how about splitting `VulkanModuleNode` and `VulkanDeviceAPI` etc into their own file?
--
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] Lunderberg commented on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-850734600
@bellkin , you are absolutely correct that it does break use of libtvm_runtime, and that was not my intention to do so. Thank you for the report on it, and I'll uptime the implementation to avoid the dependency.
--
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] bellkin commented on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
bellkin commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-850330177
using IntImm will cause link error in libtvm_runtime, because IntImm is in libtvm
--
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] Lunderberg commented on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-848245236
Whoops, thank you. Linting now passes locally, and errors should be fixed 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] Lunderberg edited a comment on pull request #8127: [Vulkan] Add device capabilities to Target, use in codegen
Posted by GitBox <gi...@apache.org>.
Lunderberg edited a comment on pull request #8127:
URL: https://github.com/apache/tvm/pull/8127#issuecomment-850734600
@bellkin , you are absolutely correct that it does break use of libtvm_runtime, and that was not my intention to do so. Thank you for the report on it, and I'll update the implementation to avoid the dependency.
--
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