Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Vulkan] Broke out implicit device requirements into SPIRVSupport #8048

Merged
merged 1 commit into from May 18, 2021

Conversation

Lunderberg
Copy link
Contributor

Codifies the current requirements that are implicit in the shaders
built by CodeGenSPIRV (e.g. can read from 8-bit buffers). The next
steps for this development are (1) to query driver/device support
information from the device, (2) to pass these query parameters
through the Target, and (3) to ensure correct shader generation even
when features are not supported.

Step (3) will require exposing the target properties to relay
optimization passes.

@Lunderberg Lunderberg force-pushed the vulkan_explicit_device_requirements branch from 25c0dad to 6a13b44 Compare May 14, 2021 15:58
@Lunderberg
Copy link
Contributor Author

Potential reviewers: @masahi @tqchen

@Lunderberg
Copy link
Contributor Author

@masahi In the new SPIRVSupport struct, there's a field that will hold the subgroup operation support. Can you confirm that this is the appropriate parameter you'll need for the prefix sum improvements?

* then this value will be set to 0.
*
*/
uint32_t supportedSubgroupOperations{0};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: stl_style to be consistent with the rest of the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, and I will change the names. I had been attempting to have the names here mimic the names of the spirv/vulkan features that they enable, but I can see how that would be inconsistent elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Member variable names have been updated, along with adding additional documentation on where they should be derived from on the vulkan side.

namespace codegen {

/*! \brief Represents which support a Vulkan driver has that are relevant to codegen */
struct SPIRVSupport {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to keep in mind is the need of cross compilation, e.g. the machine that runs compilation may not be the machine that executes the program. So we cannot do simple live query on the driver on the same machine.

It would be good to list the some attributes of interest and register them directly in the vulkan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and that is my plan for the next steps. My plan is to have many of these parameters be added to the TVM_REGISTER_TARGET_KIND for vulkan, both to pass them through to the codegen, and in the future to expose them to the optimization passes.

@Lunderberg Lunderberg force-pushed the vulkan_explicit_device_requirements branch from 6a13b44 to 09a961d Compare May 14, 2021 18:39
@masahi
Copy link
Member

masahi commented May 14, 2021

@masahi In the new SPIRVSupport struct, there's a field that will hold the subgroup operation support. Can you confirm that this is the appropriate parameter you'll need for the prefix sum improvements?

Probably we need more fine-grained information than yes/no result. Vulkan has (surprisingly) wide variety of subgroup op support as described in https://www.khronos.org/blog/vulkan-subgroup-tutorial. For prefix scan, it is probably enough to have GL_KHR_shader_subgroup_arithmetic, since it literally has subgroupExclusiveAdd. I'm also planning to do FFT at some point, for that I probably want explicit shuffle ops as provided by GL_KHR_shader_subgroup_shuffle extension.

Since this is a complicated issue, we can add support for them on a need basis later. So feel free to drop supported_subgroup_operations from this PR.

@masahi masahi self-assigned this May 14, 2021
@Lunderberg
Copy link
Contributor Author

Probably we need more fine-grained information than yes/no result. Vulkan has (surprisingly) wide variety of subgroup op support as described in https://www.khronos.org/blog/vulkan-subgroup-tutorial.

I think the bitflags from the supportedOperations member of VkPhysicalDeviceSubgroupProperties should give that more fine-grained information. The supported_subgroup_operations is intended to be a copy of the bitflags from supportedOperations, which can then be checked by the codegen. In the case of the subgroup arithmetic operations, that should be doable by checking the VK_SUBGROUP_FEATURE_ARITHMETIC_BIT of supported_subgroup_operations.

Copy link
Member

@masahi masahi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, there was a CI issue, please retrigger

Codifies the current requirements that are implicit in the shaders
built by CodeGenSPIRV (e.g. can read from 8-bit buffers).  The next
steps for this development are (1) to query driver/device support
information from the device, (2) to pass these query parameters
through the Target, and (3) to ensure correct shader generation even
when features are not supported.

Step (3) will require exposing the target properties to relay
optimization passes.
@Lunderberg Lunderberg force-pushed the vulkan_explicit_device_requirements branch from 09a961d to 43db690 Compare May 18, 2021 14:42
@tqchen tqchen merged commit 0192750 into apache:main May 18, 2021
@Lunderberg Lunderberg deleted the vulkan_explicit_device_requirements branch May 25, 2021 16:00
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 17, 2021
…ache#8048)

Codifies the current requirements that are implicit in the shaders
built by CodeGenSPIRV (e.g. can read from 8-bit buffers).  The next
steps for this development are (1) to query driver/device support
information from the device, (2) to pass these query parameters
through the Target, and (3) to ensure correct shader generation even
when features are not supported.

Step (3) will require exposing the target properties to relay
optimization passes.

Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 17, 2021
…ache#8048)

Codifies the current requirements that are implicit in the shaders
built by CodeGenSPIRV (e.g. can read from 8-bit buffers).  The next
steps for this development are (1) to query driver/device support
information from the device, (2) to pass these query parameters
through the Target, and (3) to ensure correct shader generation even
when features are not supported.

Step (3) will require exposing the target properties to relay
optimization passes.

Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
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.

None yet

3 participants