dmlc / dlpack

common in-memory tensor structure
https://dmlc.github.io/dlpack/latest
Apache License 2.0
890 stars 135 forks source link

Give DLDeviceType a sentinel value #111

Open cconvey opened 1 year ago

cconvey commented 1 year ago

The DLDeviceType enum enumerates the list of device types.

The TVM project defines another enumeration, TVMDeviceExtType, that provides a supplemental set of devices / enumerators. It's important that there's no overlap of the integers provided by DLDeviceType and TVMDeviceExtType.

Unfortunately there's currently no good mechanism to notice when changes to either project lead to both using the same integer value in those enumerations.

We could address this by adding a sentinel value to DLDeviceType, e.g.:

typedef enum {
  kDLDeviceType_Begin = 1,
  kDLCPU = kDLDeviceType_Begin,
   ...
  kDLWebGPU = 15,
  /*! \brief Qualcomm Hexagon DSP */
  kDLHexagon = 16,
  kDLDeviceType_End, // all DLDeviceType enumerators are guaranteed to be numerically lower than this integer
} DLDeviceType;   

With this in place, TVM could safely avoid problems using something like this:

typedef enum {
  kDLAOCL = kDLDeviceType_End,
  kDLSDAccel,
  kOpenGL,
  kDLMicroDev,
  kDLWebGPU,
  // AddExtraTVMType which is not in DLPack here
} TVMDeviceExtType;

or this:


typedef enum {
  kDLAOCL = ...,
  ...
} TVMDeviceExtType;

// Relies on kDLAOCL having the lowest integer value in TVMDeviceExtType.
static_assert(KDLAOCL > kDLEnumEnd);
``
cconvey commented 1 year ago

Update: I noticed that TVM also assumes that:

Probably the right solution is for TVM to stop making such strong assumptions about this enumeration, especially in code that needs to compile as C rather than C++.

But if DLPack wants to help TVM avoid problems, then this enumeration should provide both kDLDeviceType_Begin and kDLDeviceType_End.