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/14 15:52:11 UTC
[GitHub] [tvm] Lunderberg opened a new pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Lunderberg opened a new pull request #8048:
URL: https://github.com/apache/tvm/pull/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.
--
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 #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#issuecomment-841335795
@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?
--
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] tqchen commented on a change in pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#discussion_r632643129
##########
File path: src/target/spirv/spirv_support.h
##########
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file spirv_support
+ *
+ * \brief Utility for determining which spirv capabilities a TVM
+ * target supports.
+ */
+#ifndef TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+#define TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+
+#include <tvm/target/target.h>
+
+namespace tvm {
+namespace codegen {
+
+/*! \brief Represents which support a Vulkan driver has that are relevant to codegen */
+struct SPIRVSupport {
Review comment:
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.
--
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 a change in pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#discussion_r632707629
##########
File path: src/target/spirv/spirv_support.h
##########
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file spirv_support
+ *
+ * \brief Utility for determining which spirv capabilities a TVM
+ * target supports.
+ */
+#ifndef TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+#define TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+
+#include <tvm/target/target.h>
+
+namespace tvm {
+namespace codegen {
+
+/*! \brief Represents which support a Vulkan driver has that are relevant to codegen */
+struct SPIRVSupport {
+ /*! \brief Determine spirv capabilities from a vulkan target.
+ */
+ explicit SPIRVSupport(Target target);
+
+ /*!
+ * \brief The supported operations
+ *
+ * Vulkan extension: VK_KHR_driver_properties
+ * Minimum vulkan version: 1.1
+ *
+ * The supportedOperations bitflags from
+ * VkPhysicalDeviceSubgroupProperties.
+ *
+ * Requires vulkan 1.1 or higher to use. If the
+ * VK_KHR_driver_properties extension is not present in order to
+ * query this value, or if the driver does not support vulkan 1.0,
+ * then this value will be set to 0.
+ *
+ */
+ uint32_t supportedSubgroupOperations{0};
Review comment:
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.
--
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] tqchen merged pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #8048:
URL: https://github.com/apache/tvm/pull/8048
--
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] tqchen commented on a change in pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#discussion_r632640332
##########
File path: src/target/spirv/spirv_support.h
##########
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file spirv_support
+ *
+ * \brief Utility for determining which spirv capabilities a TVM
+ * target supports.
+ */
+#ifndef TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+#define TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+
+#include <tvm/target/target.h>
+
+namespace tvm {
+namespace codegen {
+
+/*! \brief Represents which support a Vulkan driver has that are relevant to codegen */
+struct SPIRVSupport {
+ /*! \brief Determine spirv capabilities from a vulkan target.
+ */
+ explicit SPIRVSupport(Target target);
+
+ /*!
+ * \brief The supported operations
+ *
+ * Vulkan extension: VK_KHR_driver_properties
+ * Minimum vulkan version: 1.1
+ *
+ * The supportedOperations bitflags from
+ * VkPhysicalDeviceSubgroupProperties.
+ *
+ * Requires vulkan 1.1 or higher to use. If the
+ * VK_KHR_driver_properties extension is not present in order to
+ * query this value, or if the driver does not support vulkan 1.0,
+ * then this value will be set to 0.
+ *
+ */
+ uint32_t supportedSubgroupOperations{0};
Review comment:
note: stl_style to be consistent with the rest of the codebase
--
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 a change in pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#discussion_r632778226
##########
File path: src/target/spirv/ir_builder.cc
##########
@@ -186,13 +215,14 @@ Value IRBuilder::FloatImm(const SType& dtype, double value) {
Value IRBuilder::BufferArgument(const SType& value_type, uint32_t descriptor_set,
uint32_t binding) {
- // NOTE: BufferBlock was deprecated in SPIRV 1.3
- // use StorageClassStorageBuffer instead.
-#if SPV_VERSION >= 0x10300
- spv::StorageClass storage_class = spv::StorageClassStorageBuffer;
-#else
- spv::StorageClass storage_class = spv::StorageClassUniform;
-#endif
+ // If SPIRV 1.3+, or with extension SPV_KHR_storage_buffer_storage_class, BufferBlock is
+ // deprecated.
+ spv::StorageClass storage_class;
+ if (spirv_support_.supports_storage_buffer_storage_class) {
+ storage_class = spv::StorageClassStorageBuffer;
+ } else {
+ storage_class = spv::StorageClassUniform;
+ }
Review comment:
Does this mean newer spirv uses uniform buffer exclusively? Is that ok given the limit on uniform buffer size.
--
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 #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#issuecomment-841527815
> 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](https://www.khronos.org/registry/vulkan/specs/1.2-extensions/man/html/VkPhysicalDeviceSubgroupProperties.html) 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](https://www.khronos.org/registry/vulkan/specs/1.2-extensions/man/html/VkSubgroupFeatureFlagBits.html) of `supported_subgroup_operations`.
--
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 a change in pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#discussion_r632724361
##########
File path: src/target/spirv/spirv_support.h
##########
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file spirv_support
+ *
+ * \brief Utility for determining which spirv capabilities a TVM
+ * target supports.
+ */
+#ifndef TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+#define TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+
+#include <tvm/target/target.h>
+
+namespace tvm {
+namespace codegen {
+
+/*! \brief Represents which support a Vulkan driver has that are relevant to codegen */
+struct SPIRVSupport {
+ /*! \brief Determine spirv capabilities from a vulkan target.
+ */
+ explicit SPIRVSupport(Target target);
+
+ /*!
+ * \brief The supported operations
+ *
+ * Vulkan extension: VK_KHR_driver_properties
+ * Minimum vulkan version: 1.1
+ *
+ * The supportedOperations bitflags from
+ * VkPhysicalDeviceSubgroupProperties.
+ *
+ * Requires vulkan 1.1 or higher to use. If the
+ * VK_KHR_driver_properties extension is not present in order to
+ * query this value, or if the driver does not support vulkan 1.0,
+ * then this value will be set to 0.
+ *
+ */
+ uint32_t supportedSubgroupOperations{0};
Review comment:
Member variable names have been updated, along with adding additional documentation on where they should be derived from on the vulkan side.
--
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 #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#issuecomment-841481729
> @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.
--
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 a change in pull request #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#discussion_r632691160
##########
File path: src/target/spirv/spirv_support.h
##########
@@ -0,0 +1,220 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file spirv_support
+ *
+ * \brief Utility for determining which spirv capabilities a TVM
+ * target supports.
+ */
+#ifndef TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+#define TVM_TARGET_SPIRV_SPIRV_SUPPORT_H_
+
+#include <tvm/target/target.h>
+
+namespace tvm {
+namespace codegen {
+
+/*! \brief Represents which support a Vulkan driver has that are relevant to codegen */
+struct SPIRVSupport {
Review comment:
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.
--
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 #8048: [Vulkan] Broke out implicit device requirements into SPIRVSupport
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8048:
URL: https://github.com/apache/tvm/pull/8048#issuecomment-841334742
Potential reviewers: @masahi @tqchen
--
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