[DLPack][runtime] Update DLPack to v0.7 #13177

cconvey · 2022-10-23T21:50:14Z

[DLPack][runtime] Update DLPack to v0.7

Update the 3rdparty/dlpack git submodule from v0.5 to v0.7, so that the DLDeviceType enumeration has an explicitly-stated underlying storage type. This addresses a compiler warning generated by clang 15.0.3.
Remove kDLHexagon and kDLWebGPU from TVMDeviceExtType, because those enumerators are now provided by DLDeviceType.
Renumber the members of TVMDeviceExtType to reduce the chance of unnoticed collision with members of DLDeviceType.

tvm-bot · 2022-10-23T21:50:17Z

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

cc @areusch _{See #10317 for details}
Built docs for commit 3c9443b can be found here.

_{Generated by tvm-bot}

cconvey · 2022-10-23T21:52:35Z

CC: @areusch @tqchen @kparzysz-quic

kparzysz-quic

Cool! Thanks.

cconvey · 2022-10-24T17:16:50Z

@tvm-bot rerun

cconvey · 2022-10-31T00:11:01Z

@tvm-bot rerun

cconvey · 2022-10-31T15:10:56Z

FYI CC: @Lunderberg @csullivan

csullivan

Many thanks @cconvey! Providing one suggestion that could help for future maintenance.

csullivan · 2022-10-31T15:40:06Z

python/tvm/_ffi/runtime_ctypes.py

+    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
+


Is there a way for us to initialize these from there corresponding DLDeviceType and TVMDeviceExtType enums potentially via a packed function call?

Is there a way for us to initialize these from there corresponding DLDeviceType and TVMDeviceExtType enums potentially via a packed function call?

I definitely considered that idea. The main goal of the PR is to upgrade DLPack, and I felt a significant reworking of how these constants are handled belonged in a separate PR.

csullivan · 2022-10-31T15:43:26Z

include/tvm/runtime/c_runtime_api.h

+ * meant to work as C code.  So the unspecified storage type
+ * could be a latent bug when compiled as C.


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.

Sorry that the comment wasn't clear!

The two header files in question, dlpack.h and 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 and here).

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.

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.

csullivan · 2022-10-31T15:44:26Z

jvm/core/src/main/java/org/apache/tvm/Device.java

+  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;
+


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.

I completely agree that TVM should have some mechanism to let these integers be defined in only one place. I think a proper solution would require some discussion, so I'd prefer to treat that as outside the scope of this PR.

- Update the `3rdparty/dlpack` git submodule from v0.5 to v0.7, so that the `DLDeviceType` enumeration has an explicitly-stated underlying storage type. This addresses a compiler warning generated by clang 15.0.3. - Remove `kDLHexagon` and `kDLWebGPU` from `TVMDeviceExtType`, because those enumerators are now provided by `DLDeviceType`. - Renumber the members of `TVMDeviceExtType` to reduce the chance of unnoticed collision with members of `DLDeviceType`.

cconvey · 2022-10-31T23:55:17Z

@tvm-bot rerun

csullivan · 2022-11-01T05:25:00Z

It's great to have DLPack updated, thank you for working through the issues @cconvey! This is now merged.

- Update the `3rdparty/dlpack` git submodule from v0.5 to v0.7, so that the `DLDeviceType` enumeration has an explicitly-stated underlying storage type. This addresses a compiler warning generated by clang 15.0.3. - Remove `kDLHexagon` and `kDLWebGPU` from `TVMDeviceExtType`, because those enumerators are now provided by `DLDeviceType`. - Renumber the members of `TVMDeviceExtType` to reduce the chance of unnoticed collision with members of `DLDeviceType`.

cconvey force-pushed the use-dlpack-v0.7 branch from a0a48fd to fa9cc08 Compare October 23, 2022 21:51

cconvey changed the title ~~[DLPack] Update DLPack to v0.7~~ [DLPack][runtime] Update DLPack to v0.7 Oct 23, 2022

kparzysz-quic approved these changes Oct 24, 2022

View reviewed changes

cconvey force-pushed the use-dlpack-v0.7 branch from fa9cc08 to 90ed6f8 Compare October 24, 2022 13:08

tqchen approved these changes Oct 24, 2022

View reviewed changes

cconvey force-pushed the use-dlpack-v0.7 branch 2 times, most recently from 6aac36f to ebf0cde Compare October 24, 2022 13:43

cconvey force-pushed the use-dlpack-v0.7 branch 3 times, most recently from 0ce1349 to 3914140 Compare October 27, 2022 20:22

shingjan approved these changes Oct 27, 2022

View reviewed changes

cconvey force-pushed the use-dlpack-v0.7 branch 6 times, most recently from 5267bbc to 965a51d Compare October 30, 2022 19:05

csullivan reviewed Oct 31, 2022

View reviewed changes

cconvey mentioned this pull request Oct 31, 2022

[CI] Android CI depends on sunsetted jcenter repo #13240

Closed

cconvey force-pushed the use-dlpack-v0.7 branch from 965a51d to 3c9443b Compare October 31, 2022 20:37

csullivan merged commit 9cdc97f into apache:main Nov 1, 2022

cconvey deleted the use-dlpack-v0.7 branch November 1, 2022 14:08

leandron mentioned this pull request Feb 1, 2023

TVM v0.11.0 Release Candidate Notes #13899

Closed

Johnson9009 mentioned this pull request Feb 24, 2023

[Runtime] Fix Potential DeviceAPIManager Memory Bug #14114

Merged

		* meant to work as C code. So the unspecified storage type
		* could be a latent bug when compiled as C.

[DLPack][runtime] Update DLPack to v0.7 #13177

[DLPack][runtime] Update DLPack to v0.7 #13177

Uh oh!

Conversation

cconvey commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvm-bot commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cconvey commented Oct 23, 2022

Uh oh!

kparzysz-quic left a comment

Choose a reason for hiding this comment

Uh oh!

cconvey commented Oct 24, 2022

Uh oh!

cconvey commented Oct 31, 2022

Uh oh!

cconvey commented Oct 31, 2022

Uh oh!

csullivan left a comment

Choose a reason for hiding this comment

Uh oh!

csullivan Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

cconvey Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

csullivan Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

cconvey Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

csullivan Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

cconvey Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

cconvey commented Oct 31, 2022

Uh oh!

csullivan commented Nov 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cconvey commented Oct 23, 2022 •

edited

Loading

tvm-bot commented Oct 23, 2022 •

edited

Loading