cconvey commented on code in PR #13177:
URL: https://github.com/apache/tvm/pull/13177#discussion_r1009848023
##########
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:
Sorry that the comment wasn't clear!
The two header files in question,
[`dlpack.h`](https://github.com/dmlc/dlpack/blob/e2bdd3bee8cb6501558042633fa59144cc8b7f5f/include/dlpack/dlpack.h)
and
[`include/tvm/runtime/c_runtime_api.h`](https://github.com/apache/tvm/blob/965a51de0b6e515fe0d5a34fba3b8b1ebf69aeb4/include/tvm/runtime/c_runtime_api.h),
are meant to be valid C as well as valid C++.
When compiled as C++, these header files _force_ the `DLDeviceType` and
`TVMDeviceExtType` enums to use `int32_t` as the underlying storage type. (See
[here](https://github.com/dmlc/dlpack/blob/e2bdd3bee8cb6501558042633fa59144cc8b7f5f/include/dlpack/dlpack.h#L44-L48)
and
[here](https://github.com/apache/tvm/blob/965a51de0b6e515fe0d5a34fba3b8b1ebf69aeb4/include/tvm/runtime/c_runtime_api.h#L106-L110)).
This ensures that any variable of type `DLDeviceType` or `TVMDeviceExtType`
is capable of storing any of the values from those two enumerations. (And,
fortunately, any of the additional values that TVM assumes it can store there,
such as `0` and `-1`.)
But when those headers are compiled as _C_ code, those enum declarations
_don't_ have that `int32_t` specifier. IIUC, C11 (the newest standard version
of C, I think) [has no such mechanism for specifying the size of an
enum](https://stackoverflow.com/questions/4879286/specifying-size-of-enum-type-in-c).
I believe this means the compiler is permitted to pick any underlying
storage type for `DLDeviceType` vs. `TVMDeviceExtType` that can accommodate the
enums' respective values. So, for example, `int8_t` for one, `uint32_t` for
the other.
Practically speaking, we'd expect the compiler(s) to pick the same storage
type for both `DLDeviceType` and `TVMDeviceExtType`, because currently they
both have only non-negative integers pretty close to 0. But the compiler
_could_, in principle, use one underlying storage type when compiling DLPack,
and another when compiling TVM, possibly leading to an ABI mismatch. So this
is the latent bug to which I referred.
Another consideration is that TVM has code that assumes variables of (one
of) these types can store the value -1. I think this is undefined behavior,
and could potentially cause problems with code that compares / tests the values
of these variables.
--
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]
