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/04/16 17:30:09 UTC
[GitHub] [tvm] Lunderberg opened a new pull request #7867: [Runtime] Driver version + consistent clock speed units
Lunderberg opened a new pull request #7867:
URL: https://github.com/apache/tvm/pull/7867
A few small changes to on the DeviceAPI side.
- Added kDriverVersion as an option for GetAttr. Some of the vulkan tests seem to have driver-specific deltas, so this will help collect information. Exposed this through `tvm._ffi.runtime_ctypes.Device` class.
- Added `api_version` to python `Device` class, which was previously query-able on C++ side.
- Updates to docstrings in `Device` to clarify differences between `compute_version`, `api_version`, and `driver_version`.
- Updated clock speed returned by OpenCL runtime to be in kHz, consistent with units returned by cuda/rocm runtimes.
--
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] areusch commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r615279945
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -262,6 +264,35 @@ def max_thread_dimensions(self):
"""
return json.loads(self._GetDeviceAttr(self.device_type, self.device_id, 8))
+ @property
+ def api_version(self):
+ """Returns version number of the SDK used to compile TVM.
+
+ For example, CUDA_VERSION for cuda or VK_HEADER_VERSION for
+ Vulkan.
+
+ Returns
+ -------
+ version : int
+ The version of the SDK
+
+ """
+ return self._GetDeviceAttr(self.device_type, self.device_id, 12)
+
+ @property
+ def driver_version(self):
+ """Returns version number of the driver
+
+ Returns driver vendor's internal version number.
+ (e.g. "450.408.256" for nvidia-driver-450)
+
+ Returns
+ -------
+ version : str
Review comment:
seems like some implementations return nullptr/None, want to document when and why?
--
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] areusch commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616186396
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
+ const size_t version_start = 7;
Review comment:
ok cool, yeah either that or just add comment would be great!
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#issuecomment-821798479
@Lunderberg Seems CI is stuck, need to kick again. Also there is a conflict.
--
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] areusch commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616187531
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
Review comment:
er, I haven't tried this but what's checked-in at main looks like it returns `clGetDeviceInfo(..., CL_DEVICE_NAME, ...)` and this change strips any suffix past a `' '`. am I missing something here?
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616195716
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
Review comment:
Ooh, I think I see the confusion. The current commit on main queries `CL_DEVICE_NAME` for `kDeviceName`, but returns immediately for `kComputeVersion`. The PR here would still query `CL_DEVICE_NAME` for `kDeviceName`, but has the suffix-stripping for `kComputeVersion`.
The handling of `kDeviceName` is a few lines below, lines 88-90 of the modified file. I noticed that it could take advantage of the `GetDeviceInfo` helper function that was already in this file, which is why it looks a bit different than in the previous. It still shows the full device name without any modification (e.g. "GeForce GTX 1080" on my machine)
```
device_type cpu gpu opencl vulkan
attribute
kApiVersion None 10010 220 131
kComputeVersion None 6.1 1.2 1.2.133
kDeviceName None GeForce GTX 1080 GeForce GTX 1080 GeForce GTX 1080
kDriverVersion None None 450.102.04 450.408.256
kGcnArch None None None None
kMaxClockRate None 1809500 1809000 None
kMaxRegistersPerBlock None 65536 None None
kMaxSharedMemoryPerBlock None 49152 49152 49152
kMaxThreadDimensions None [1024, 1024, 64] [1024, 1024, 64] [1536, 1024, 64]
kMaxThreadsPerBlock None 1024 1024 1536
kMultiProcessorCount None 20 20 None
kWarpSize None 32 1 32
```
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#issuecomment-823426181
One more commit to address the CI failure. I had been using the newer unified opencl header files, where the CI has the earlier version-specific header 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] masahi commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r615163384
##########
File path: src/runtime/vulkan/vulkan.cc
##########
@@ -487,15 +488,29 @@ void VulkanDeviceAPI::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
break;
}
case kMaxRegistersPerBlock:
- return;
+ break;
case kGcnArch:
- return;
+ break;
case kApiVersion:
- return;
+ *rv = VK_HEADER_VERSION;
+ break;
+ case kDriverVersion: {
+ int64_t value = phy_prop.driverVersion;
+ std::ostringstream os;
+ os << VK_VERSION_MAJOR(value) << "." << VK_VERSION_MINOR(value) << "."
+ << VK_VERSION_PATCH(value);
+ *rv = os.str();
+ break;
+ }
}
}
VulkanDeviceAPI::VulkanDeviceAPI() {
+ uint32_t version;
+ vkEnumerateInstanceVersion(&version);
+ std::cout << "Vulkan instance version " << VK_VERSION_MAJOR(version) << "."
Review comment:
LOG(INFO)
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#issuecomment-821397971
Potential reviewers: @hogepodge for the documentation changes, or @masahi / @areusch / @tmoreau89 for runtime changes.
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616181202
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
+ const size_t version_start = 7;
Review comment:
7 characters skips past the initial "OpenCL " at the start of the version returned by OpenCL. (e.g. "OpenCL 1.2 CUDA" is returned on my machine). I can make it more explicit as `strlen("OpenCL ")` instead.
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #7867:
URL: https://github.com/apache/tvm/pull/7867
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#issuecomment-824194789
@areusch Good point, and that is useful feedback. Changes made in each docstring to indicate that they can return `None`. The one exception is `.exist`, which returns a boolean for all implementations.
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616184276
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -262,6 +264,35 @@ def max_thread_dimensions(self):
"""
return json.loads(self._GetDeviceAttr(self.device_type, self.device_id, 8))
+ @property
+ def api_version(self):
+ """Returns version number of the SDK used to compile TVM.
+
+ For example, CUDA_VERSION for cuda or VK_HEADER_VERSION for
+ Vulkan.
+
+ Returns
+ -------
+ version : int
+ The version of the SDK
+
+ """
+ return self._GetDeviceAttr(self.device_type, self.device_id, 12)
+
+ @property
+ def driver_version(self):
+ """Returns version number of the driver
+
+ Returns driver vendor's internal version number.
+ (e.g. "450.408.256" for nvidia-driver-450)
+
+ Returns
+ -------
+ version : str
Review comment:
Good point, I had been thinking of implemented in C++ virtual functions terms. I'll update the docstrings to instead read "Returns device value for X, Y, and Z devices. Returns None for any other devices."
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#issuecomment-822749420
Thank you, @masahi . Looks like the conflict was due to additional rebasing needed.
@areusch , I've added additional documentation on the `.driver_version` property, along with more detailed documentation on each property in `Device`.
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#issuecomment-824518257
thanks @Lunderberg @areusch @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] hogepodge commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
hogepodge commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616954496
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -205,63 +214,190 @@ def _GetDeviceAttr(self, device_type, device_id, attr_id):
@property
def exist(self):
- """Whether this device exist."""
+ """Whether this device exists.
+
+ Returns True if TVM was support for the device, if the
Review comment:
s/was/has
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -205,63 +214,190 @@ def _GetDeviceAttr(self, device_type, device_id, attr_id):
@property
def exist(self):
- """Whether this device exist."""
+ """Whether this device exists.
+
+ Returns True if TVM was support for the device, if the
+ physical device is present, and the device is accessible
+ through appropriate drivers (e.g. cuda/vulkan).
+
+ Returns
+ -------
+ exist : bool
+ Whether the device exists
Review comment:
s/Whether/True if
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r615174660
##########
File path: src/runtime/vulkan/vulkan.cc
##########
@@ -487,15 +488,29 @@ void VulkanDeviceAPI::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
break;
}
case kMaxRegistersPerBlock:
- return;
+ break;
case kGcnArch:
- return;
+ break;
case kApiVersion:
- return;
+ *rv = VK_HEADER_VERSION;
+ break;
+ case kDriverVersion: {
+ int64_t value = phy_prop.driverVersion;
+ std::ostringstream os;
+ os << VK_VERSION_MAJOR(value) << "." << VK_VERSION_MINOR(value) << "."
+ << VK_VERSION_PATCH(value);
+ *rv = os.str();
+ break;
+ }
}
}
VulkanDeviceAPI::VulkanDeviceAPI() {
+ uint32_t version;
+ vkEnumerateInstanceVersion(&version);
+ std::cout << "Vulkan instance version " << VK_VERSION_MAJOR(version) << "."
Review comment:
Thank you for catching that. Removed, and PR is updated.
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616992084
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -205,63 +214,190 @@ def _GetDeviceAttr(self, device_type, device_id, attr_id):
@property
def exist(self):
- """Whether this device exist."""
+ """Whether this device exists.
+
+ Returns True if TVM was support for the device, if the
Review comment:
Thank you, typo fixed.
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -205,63 +214,190 @@ def _GetDeviceAttr(self, device_type, device_id, attr_id):
@property
def exist(self):
- """Whether this device exist."""
+ """Whether this device exists.
+
+ Returns True if TVM was support for the device, if the
+ physical device is present, and the device is accessible
+ through appropriate drivers (e.g. cuda/vulkan).
+
+ Returns
+ -------
+ exist : bool
+ Whether the device exists
Review comment:
Thank you, updated.
--
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] areusch commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616170835
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
+ const size_t version_start = 7;
Review comment:
why is it 7?
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
Review comment:
hmm, this seems a bit like a breaking change. should we just document that the version string always starts with "major.minor" and may be backend-specific?
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -262,6 +264,35 @@ def max_thread_dimensions(self):
"""
return json.loads(self._GetDeviceAttr(self.device_type, self.device_id, 8))
+ @property
+ def api_version(self):
+ """Returns version number of the SDK used to compile TVM.
+
+ For example, CUDA_VERSION for cuda or VK_HEADER_VERSION for
+ Vulkan.
+
+ Returns
+ -------
+ version : int
+ The version of the SDK
+
+ """
+ return self._GetDeviceAttr(self.device_type, self.device_id, 12)
+
+ @property
+ def driver_version(self):
+ """Returns version number of the driver
+
+ Returns driver vendor's internal version number.
+ (e.g. "450.408.256" for nvidia-driver-450)
+
+ Returns
+ -------
+ version : str
Review comment:
could you be more explicit in the docstring? imo, "Implemented for opencl and vulkan" would read to a Python developer as "raises NotImplementedError when not opencl or 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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616178257
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
Review comment:
I'm afraid I don't quite see the breakage at this part. For the OpenCLWorkspace, previously any request for kComputeVersion would return nullptr/None. If there were a return value in previous versions, I'd agree that we'd want to avoid breaking it, but I think for a first implementation we're free to maintain consistency with other DeviceAPI implementations.
--
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] areusch commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r617219695
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -262,6 +264,35 @@ def max_thread_dimensions(self):
"""
return json.loads(self._GetDeviceAttr(self.device_type, self.device_id, 8))
+ @property
+ def api_version(self):
+ """Returns version number of the SDK used to compile TVM.
+
+ For example, CUDA_VERSION for cuda or VK_HEADER_VERSION for
+ Vulkan.
+
+ Returns
+ -------
+ version : int
+ The version of the SDK
+
+ """
+ return self._GetDeviceAttr(self.device_type, self.device_id, 12)
+
+ @property
+ def driver_version(self):
+ """Returns version number of the driver
+
+ Returns driver vendor's internal version number.
+ (e.g. "450.408.256" for nvidia-driver-450)
+
+ Returns
+ -------
+ version : str
Review comment:
see also https://numpydoc.readthedocs.io/en/latest/format.html#sections, which doesn't require you to name return values; this might help with adding multiple types of return values.
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r615947927
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -262,6 +264,35 @@ def max_thread_dimensions(self):
"""
return json.loads(self._GetDeviceAttr(self.device_type, self.device_id, 8))
+ @property
+ def api_version(self):
+ """Returns version number of the SDK used to compile TVM.
+
+ For example, CUDA_VERSION for cuda or VK_HEADER_VERSION for
+ Vulkan.
+
+ Returns
+ -------
+ version : int
+ The version of the SDK
+
+ """
+ return self._GetDeviceAttr(self.device_type, self.device_id, 12)
+
+ @property
+ def driver_version(self):
+ """Returns version number of the driver
+
+ Returns driver vendor's internal version number.
+ (e.g. "450.408.256" for nvidia-driver-450)
+
+ Returns
+ -------
+ version : str
Review comment:
Certainly, can do. It looks like several of the other parameters also return None for some implementations, sometimes for cases where a parameter doesn't make sense (e.g. warp size for cpu targets), or API limitations (e.g. clock speed on vulkan). I'll see if I can flesh out some of the implementations, and add documentation for each parameter on when they return None.
--
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 #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r616199114
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -262,6 +264,35 @@ def max_thread_dimensions(self):
"""
return json.loads(self._GetDeviceAttr(self.device_type, self.device_id, 8))
+ @property
+ def api_version(self):
+ """Returns version number of the SDK used to compile TVM.
+
+ For example, CUDA_VERSION for cuda or VK_HEADER_VERSION for
+ Vulkan.
+
+ Returns
+ -------
+ version : int
+ The version of the SDK
+
+ """
+ return self._GetDeviceAttr(self.device_type, self.device_id, 12)
+
+ @property
+ def driver_version(self):
+ """Returns version number of the driver
+
+ Returns driver vendor's internal version number.
+ (e.g. "450.408.256" for nvidia-driver-450)
+
+ Returns
+ -------
+ version : str
Review comment:
Also, as long as I am looking at it, it would be good to explicitly state that the RPC devices return the value of the remote device.
--
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] areusch commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r617218628
##########
File path: src/runtime/opencl/opencl_device_api.cc
##########
@@ -72,20 +75,26 @@ void OpenCLWorkspace::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
*rv = static_cast<int64_t>(value);
break;
}
- case kComputeVersion:
- return;
- case kDeviceName: {
- char value[128] = {0};
- OPENCL_CALL(
- clGetDeviceInfo(devices[index], CL_DEVICE_NAME, sizeof(value) - 1, value, nullptr));
- *rv = std::string(value);
+ case kComputeVersion: {
+ std::string ret = GetDeviceInfo(devices[index], CL_DEVICE_VERSION);
+ // String returned is "OpenCL $MAJOR.$MINOR $VENDOR_INFO". To
+ // match other implementations, we want to return "$MAJOR.$MINOR"
Review comment:
oh! gotcha, my bad. sorry this makes way more sense now. thanks for that explanation!
##########
File path: python/tvm/_ffi/runtime_ctypes.py
##########
@@ -262,6 +264,35 @@ def max_thread_dimensions(self):
"""
return json.loads(self._GetDeviceAttr(self.device_type, self.device_id, 8))
+ @property
+ def api_version(self):
+ """Returns version number of the SDK used to compile TVM.
+
+ For example, CUDA_VERSION for cuda or VK_HEADER_VERSION for
+ Vulkan.
+
+ Returns
+ -------
+ version : int
+ The version of the SDK
+
+ """
+ return self._GetDeviceAttr(self.device_type, self.device_id, 12)
+
+ @property
+ def driver_version(self):
+ """Returns version number of the driver
+
+ Returns driver vendor's internal version number.
+ (e.g. "450.408.256" for nvidia-driver-450)
+
+ Returns
+ -------
+ version : str
Review comment:
I think what you added to the description is good, but I still think the "Returns" section is confusing, because it doesn't state "str or None." Someone skimming this would likely miss the body section. Could you add the None part to "Returns," here and 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] masahi commented on a change in pull request #7867: [Runtime] Driver version + consistent clock speed units
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7867:
URL: https://github.com/apache/tvm/pull/7867#discussion_r615163384
##########
File path: src/runtime/vulkan/vulkan.cc
##########
@@ -487,15 +488,29 @@ void VulkanDeviceAPI::GetAttr(Device dev, DeviceAttrKind kind, TVMRetValue* rv)
break;
}
case kMaxRegistersPerBlock:
- return;
+ break;
case kGcnArch:
- return;
+ break;
case kApiVersion:
- return;
+ *rv = VK_HEADER_VERSION;
+ break;
+ case kDriverVersion: {
+ int64_t value = phy_prop.driverVersion;
+ std::ostringstream os;
+ os << VK_VERSION_MAJOR(value) << "." << VK_VERSION_MINOR(value) << "."
+ << VK_VERSION_PATCH(value);
+ *rv = os.str();
+ break;
+ }
}
}
VulkanDeviceAPI::VulkanDeviceAPI() {
+ uint32_t version;
+ vkEnumerateInstanceVersion(&version);
+ std::cout << "Vulkan instance version " << VK_VERSION_MAJOR(version) << "."
Review comment:
LOG(INFO) or remove it if this is left over from development
--
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