Simplify UnpackInitializerData API #8736
Conversation
| @@ -155,23 +155,23 @@ static Status GetExternalDataInfo(const ONNX_NAMESPACE::TensorProto& tensor_prot | |||
| // This function does not unpack string_data of an initializer tensor | |||
| static Status ReadExternalDataForTensor(const ONNX_NAMESPACE::TensorProto& tensor_proto, | |||
| const ORTCHAR_T* tensor_proto_dir, | |||
| std::unique_ptr<unsigned char[]>& unpacked_tensor, | |||
| SafeInt<size_t>& tensor_byte_size) { | |||
| std::vector<uint8_t>& unpacked_tensor) { | |||
There was a problem hiding this comment.
is it possible to use std::byte?
There was a problem hiding this comment.
Unfortunately, seems CUDA does not have std::byte support yet, revert back to use uint8+t
| LOGS(logger, ERROR) << "Error while unpack min tensor: " << status.ErrorMessage(); | ||
| return false; | ||
| } | ||
| min = reinterpret_cast<float*>(unpacked_tensor.data())[0]; |
There was a problem hiding this comment.
is this vector<uint8_t>::data guaranteed to be suitably aligned for floats?
There was a problem hiding this comment.
In theory the data in the vector will be aligned to std::max_align_t, which is usually 16 or maybe 8 on some 32bit system, this makes it enough for all the scalar types we support for now.
To make it more robust, (so far I don't think we have 16bit data type yet, but just in case it will be added in the future), we can change the unpacked_tensor.resize(tensor_byte_size); to something like
unpacked_tensor = std::vector<std::byte>(tensor_byte_size, custom_alligned_allocator);, can look into this later
There was a problem hiding this comment.
thanks - sounds like it should be fine for now then
| // Onnx quantization uses uint8 [int8 not yet supported], need to cast to int32_t used by NNAPI | ||
| zero_point = static_cast<int32_t>(unpacked_tensor.get()[0]); | ||
| zero_point = static_cast<int32_t>(unpacked_tensor[0]); |
There was a problem hiding this comment.
Yes, we should check the length of the buffer, but this is not the reason for the crashes
| ORT_RETURN_IF_ERROR(GetExternalDataInfo( | ||
| tensor_proto, | ||
| tensor_proto_dir, | ||
| external_file_path, | ||
| file_offset, | ||
| tensor_byte_size)); | ||
|
|
||
| unpacked_tensor.reset(new unsigned char[*&tensor_byte_size]); | ||
| unpacked_tensor.resize(tensor_byte_size); | ||
| ORT_RETURN_IF_ERROR(onnxruntime::Env::Default().ReadFileIntoBuffer( |
There was a problem hiding this comment.
Seems zero len is fine here, same in posix version
onnxruntime/onnxruntime/core/platform/windows/env.cc
Lines 255 to 256 in 2243804
Description: Simplify UnpackInitializerData API
Motivation and Context
onnxruntime/core/providers/shared/utils/utils.cc