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/03/06 06:37:32 UTC
[GitHub] [tvm] apivovarov opened a new pull request #7605: Fix RelayVM for 32-bit platforms
apivovarov opened a new pull request #7605:
URL: https://github.com/apache/tvm/pull/7605
RelayVM runtime used platform dependent type `size_t` for code serialization and hashcode calculation.
As a result compiled models did not work on 32-bit platforms such as ARMv7.
Related Discussion: https://discuss.tvm.apache.org/t/relayvm-on-armv7-fails-cannot-find-function-in-module/9309
----------------------------------------------------------------
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] apivovarov commented on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov commented on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-793081233
I checked all Save/Load methods.
`size_t` was used only for Constants (fixed)
After this fix we use the following types in each method:
- Header - uint64_t, string
- GlobalSection - string
- ConstantSection - number of constants - uint64_t, NDArray (see below) and const_device_type - uint64_t (was size_t before).
- PrimitiveOpNames - string
- CodeSection: number of functions - uint64_t, functions - string, instructions - uint64_t
During NDArray serialization we write the following to the Stream:
- cpu_ctx: DLContext - int device_type, int device_id (`int` is 4 bytes on 32 and 64 bit platforms. On some exotic platforms it can be 16 bytes)
- dtype: DLDataType - uint8_t code, uint8_t bits, uint16_t lanes
- int ndim (usually 4 bytes), int64_t* shape, data void* (data_byte_size calculation is platform independent = uint8_t bits * int64_t num_elems)
We should probably use `int32_t` instead of 'int' during the serialization (DLContext and ndim).
It is difficult to find such a device where `int` is 16 bits. Lets make this improvement in a separate PR with lower priority.
----------------------------------------------------------------
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] comaniac commented on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-794604513
Thanks @apivovarov @zhigaowu @SWu @anijain2305
----------------------------------------------------------------
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] zhiics commented on a change in pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#discussion_r589131082
##########
File path: src/runtime/vm/executable.cc
##########
@@ -525,12 +521,10 @@ void Executable::LoadConstantSection(dmlc::Stream* strm) {
}
// Load the const to device mapping.
- std::vector<size_t> const_device_type;
+ std::vector<int64_t> const_device_type;
Review comment:
to keep consistent with other places, let's just use Index for int64_t for this change, we have created the alias for it already.
----------------------------------------------------------------
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] anijain2305 commented on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-794595308
@SWu Please explicitly approve the PR if you are ok with the changes, I can merge then.
----------------------------------------------------------------
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] SWu commented on a change in pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
SWu commented on a change in pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#discussion_r589486375
##########
File path: src/runtime/vm/executable.cc
##########
@@ -525,12 +521,10 @@ void Executable::LoadConstantSection(dmlc::Stream* strm) {
}
Review comment:
do the size_t references above here also need to be patched?
----------------------------------------------------------------
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] apivovarov commented on a change in pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov commented on a change in pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#discussion_r589753492
##########
File path: src/runtime/vm/executable.cc
##########
@@ -525,12 +521,10 @@ void Executable::LoadConstantSection(dmlc::Stream* strm) {
}
// Load the const to device mapping.
- std::vector<size_t> const_device_type;
+ std::vector<int64_t> const_device_type;
Review comment:
fixed
----------------------------------------------------------------
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] apivovarov commented on a change in pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov commented on a change in pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#discussion_r589717449
##########
File path: src/runtime/vm/executable.cc
##########
@@ -525,12 +521,10 @@ void Executable::LoadConstantSection(dmlc::Stream* strm) {
}
Review comment:
Here we use `uint64_t` to read `number of constants` from the Stream.
After that we read all constants from the Stream one after another.
`constants` vector size can not be bigger that `size_t` on a particular platform.
It is ok to use `size_t` for looping purpose (`size_t` is not written or read to/from the Stream).
----------------------------------------------------------------
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] apivovarov edited a comment on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov edited a comment on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-793081233
I checked all Save/Load methods.
`size_t` was used only for Constants (fixed)
After this fix we use the following types in each method:
- Header - uint64_t, string
- GlobalSection - string
- ConstantSection - number of constants - uint64_t, NDArray (see below) and const_device_type - uint64_t (was size_t before).
- PrimitiveOpNames - string
- CodeSection: number of functions - uint64_t, functions - string, instructions - uint64_t
During NDArray serialization we write the following to the Stream:
- cpu_ctx: DLContext - int device_type, int device_id (`int` is 4 bytes on 32 and 64 bit platforms. On some exotic platforms it can be 16 bytes)
- dtype: DLDataType - uint8_t code, uint8_t bits, uint16_t lanes
- int ndim (usually 4 bytes), int64_t* shape, data void* (data_byte_size calculation is platform independent = uint8_t bits * int64_t num_elems)
We should probably use `int32_t` instead of `int` during the serialization (DLContext and ndim).
It is difficult to find such a device where `int` is 16 bits. Lets make this improvement in a separate PR with lower priority.
----------------------------------------------------------------
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] zhiics commented on a change in pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#discussion_r589131082
##########
File path: src/runtime/vm/executable.cc
##########
@@ -525,12 +521,10 @@ void Executable::LoadConstantSection(dmlc::Stream* strm) {
}
// Load the const to device mapping.
- std::vector<size_t> const_device_type;
+ std::vector<int64_t> const_device_type;
Review comment:
to keep consistent with other places, let's just use `Index` for int64_t for this change, we have created the alias for it already.
----------------------------------------------------------------
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] apivovarov edited a comment on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov edited a comment on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-793081233
I checked all Save/Load methods.
`size_t` was used only for Constants (fixed)
After this fix we use the following types in each method:
- Header - uint64_t, string
- GlobalSection - string
- ConstantSection - number of constants - uint64_t, NDArray (see below) and const_device_type - uint64_t (was size_t before).
- PrimitiveOpNames - string
- CodeSection: number of functions - uint64_t, functions - string, instructions - uint64_t
During NDArray serialization we write the following to the Stream:
- cpu_ctx: DLContext - int device_type, int device_id (`int` is 4 bytes on 32 and 64 bit platforms. On 16-bit platforms it can be 2 bytes)
- dtype: DLDataType - uint8_t code, uint8_t bits, uint16_t lanes
- int ndim (usually 4 bytes), int64_t* shape, data void* (data_byte_size calculation is platform independent = uint8_t bits * int64_t num_elems)
We should probably use `int32_t` instead of `int` during the serialization (DLContext and ndim).
`int` is 2 bytes on 16-bit OS (such as Arduino Uno). Lets make fixes for 16-bit OS in a separate PR (Not sure if TVM supports 16-bit platforms at all).
----------------------------------------------------------------
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] apivovarov edited a comment on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov edited a comment on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-793081233
I checked all Save/Load methods.
`size_t` was used only for Constants (fixed)
After this fix we use the following types in each method:
- Header - uint64_t, string
- GlobalSection - string
- ConstantSection - number of constants - uint64_t, NDArray (see below) and const_device_type - uint64_t (was size_t before).
- PrimitiveOpNames - string
- CodeSection: number of functions - uint64_t, functions - string, instructions - uint64_t
During NDArray serialization we write the following to the Stream:
- cpu_ctx: DLContext - int device_type, int device_id (`int` is 4 bytes on 32 and 64 bit platforms. On some exotic platforms it can be 16 bytes)
- dtype: DLDataType - uint8_t code, uint8_t bits, uint16_t lanes
- int ndim (usually 4 bytes), int64_t* shape, data void* (data_byte_size calculation is platform independent = uint8_t bits * int64_t num_elems)
We should probably use `int32_t` instead of `int` during the serialization (DLContext and ndim).
`int` is 2 bytes on 16-bit OS (such as Arduino Uno). Lets make fixes for 16-bit OS in a separate PR (Not sure if TVM supports 16 bit platforms at all).
----------------------------------------------------------------
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] apivovarov edited a comment on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov edited a comment on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-793081233
I checked all Save/Load methods.
`size_t` was used only for Constants (fixed)
After this fix we use the following types in each method:
- Header - uint64_t, string
- GlobalSection - string
- ConstantSection - number of constants - uint64_t, NDArray (see below) and const_device_type - uint64_t (was size_t before).
- PrimitiveOpNames - string
- CodeSection: number of functions - uint64_t, functions - string, instructions - uint64_t
During NDArray serialization we write the following to the Stream:
- cpu_ctx: DLContext - int device_type, int device_id (`int` is 4 bytes on 32 and 64 bit platforms. On some exotic platforms it can be 16 bytes)
- dtype: DLDataType - uint8_t code, uint8_t bits, uint16_t lanes
- int ndim (usually 4 bytes), int64_t* shape, data void* (data_byte_size calculation is platform independent = uint8_t bits * int64_t num_elems)
We should probably use `int32_t` instead of `int` during the serialization (DLContext and ndim).
`int` is 2 bytes on 16-bit OS (such as Arduino Uno). Lets make fixes for 16-bit OS in a separate PR (Not sure if TVM supports 16-bit platforms at all).
----------------------------------------------------------------
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] comaniac merged pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
comaniac merged pull request #7605:
URL: https://github.com/apache/tvm/pull/7605
----------------------------------------------------------------
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] apivovarov commented on a change in pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov commented on a change in pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#discussion_r589717449
##########
File path: src/runtime/vm/executable.cc
##########
@@ -525,12 +521,10 @@ void Executable::LoadConstantSection(dmlc::Stream* strm) {
}
Review comment:
Here we use `uint64_t` to read `number of constants` from the Stream.
After that we read all constants from the Stream one after another.
`constants` vector size can not be bigger that `size_t` on a particular platform.
It is ok to use `size_t` here (just for looping purpose).
----------------------------------------------------------------
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] apivovarov edited a comment on pull request #7605: Fix RelayVM for 32-bit platforms
Posted by GitBox <gi...@apache.org>.
apivovarov edited a comment on pull request #7605:
URL: https://github.com/apache/tvm/pull/7605#issuecomment-793081233
I checked all Save/Load methods.
`size_t` was used only for Constants (fixed)
After this fix we use the following types in each method:
- Header - uint64_t, string
- GlobalSection - string
- ConstantSection - number of constants - uint64_t, NDArray (see below) and const_device_type - uint64_t (was size_t before).
- PrimitiveOpNames - string
- CodeSection: number of functions - uint64_t, functions - string, instructions - uint64_t
During NDArray serialization we write the following to the Stream:
- cpu_ctx: DLContext - int device_type, int device_id (`int` is 4 bytes on 32 and 64 bit platforms. On 16-bit platforms it can be 2 bytes)
- dtype: DLDataType - uint8_t code, uint8_t bits, uint16_t lanes
- int ndim (usually 4 bytes), int64_t* shape, data void* (data_byte_size calculation is platform independent = uint8_t bits * int64_t num_elems)
We should probably use `int32_t` instead of `int` during the serialization (DLContext and ndim).
`int` is 2 bytes on 16-bit OS (such as Arduino Uno). Lets make the fixes for 16-bit OS in a separate PR (Not sure if TVM supports 16-bit platforms at all).
----------------------------------------------------------------
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