csullivan commented on code in PR #13177:
URL: https://github.com/apache/tvm/pull/13177#discussion_r1009568249
##########
python/tvm/_ffi/runtime_ctypes.py:
##########
@@ -195,44 +195,73 @@ class Device(ctypes.Structure):
OpenCL. Some properties may return None depending on whether an
API exposes that particular property.
+ NOTE! The integer values in MASK2STR and STR2MASK *must* correspond
+ to the values provided by the DLDeviceType and TVMDeviceExtType enums.
"""
+ kDLCPU = 1
+ kDLCUDA = 2
+ kDLCUDAHost = 3
+ kDLOpenCL = 4
+ kDLVulkan = 7
+ kDLMetal = 8
+ kDLVPI = 9
+ kDLROCM = 10
+ kDLROCMHost = 11
+ kDLExtDev = 12
+ kDLCUDAManaged = 13
+ kDLOneAPI = 14
+ kDLWebGPU = 15
+ kDLHexagon = 16
+ kDLAOCL = 32
+ kDLSDAccel = 33
+ kOpenGL = 34
+ kDLMicroDev = 35
+
Review Comment:
Is there a way for us to initialize these from there corresponding
DLDeviceType and TVMDeviceExtType enums potentially via a packed function call?
##########
include/tvm/runtime/c_runtime_api.h:
##########
@@ -80,17 +80,74 @@ extern "C" {
/*! \brief type of array index. */
typedef int64_t tvm_index_t;
-/*! \brief Extension device types in TVM */
+/*! \brief Extension device types in TVM
+ *
+ * Additional enumerators to supplement those provided by
+ * DLPack's `DLDeviceType` enumeration.
+ *
+ * MAINTAINERS NOTE #1: We need to ensure that the two devices
+ * are identified by the same integer.
+ * Currently this requires manual verification.
+ * Discussed here: https://github.com/dmlc/dlpack/issues/111
+ * As of DLPack v0.7, the highest-valued enumerator in
+ * `DLDeviceType` is kDLHexagon = 16.
+ *
+ * MAINTAINERS NOTE #2: As of DLPack v0.7, the definition for
+ * `DLDeviceType` specifies an underlying storage type of
+ * `int32_t`. That guarantees a variable of type
+ * `DLDeviceType` is capable of holding any integers provided
+ * by *either* of these enumerations.
+ *
+ * However, the `int32_t` specification only applies when the
+ * header file is compiled as C++, and this header file is also
+ * meant to work as C code. So the unspecified storage type
+ * could be a latent bug when compiled as C.
Review Comment:
What "unspecified" storage type is being referred to here? I didn't
understand this comment and therefore couldn't gauge its severity given the
reliance of the TVM C-runtime on this header.
##########
jvm/core/src/main/java/org/apache/tvm/Device.java:
##########
@@ -17,32 +17,40 @@
package org.apache.tvm;
-import org.apache.tvm.rpc.RPC;
-
import java.util.HashMap;
import java.util.Map;
+import org.apache.tvm.rpc.RPC;
public class Device {
+ /**
+ * Provides the same information as the C++ enums DLDeviceType and
+ * TVMDeviceExtType.
+ */
+ static final int kDLCPU = 1, kDLCUDA = 2, kDLCUDAHost = 3, kDLOpenCL = 4,
kDLVulkan = 7,
+ kDLMetal = 8, kDLVPI = 9, kDLROCM = 10, kDLROCMHost = 11,
kDLExtDev = 12,
+ kDLCUDAManaged = 13, kDLOneAPI = 14, kDLWebGPU = 15,
kDLHexagon = 16,
+ kDLAOCL = 32, kDLSDAccel = 33, kOpenGL = 34, kDLMicroDev =
35;
+
Review Comment:
Can we initialize these with a backed function call? If we have to hard code
it somewhere, better to do it in one place than in many places and make an FFI
call to lookup the 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
