-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GPU] Graph serialization for GPU #2 #13986
Conversation
8c73c78
to
71874ae
Compare
@@ -6,13 +6,13 @@ | |||
|
|||
#include "intel_gpu/graph/topology.hpp" | |||
#include "intel_gpu/graph/program.hpp" | |||
#include "intel_gpu/graph/serialization/binary_buffer.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
Moved the include path. Thank you.
throw std::runtime_error("Failed to write " + std::to_string(size) + " bytes to stream! Wrote " + std::to_string(written_size)); | ||
} | ||
OPENVINO_ASSERT(written_size == size, | ||
"Failed to write " + std::to_string(size) + " bytes to stream! Wrote " + std::to_string(written_size)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
Changed to use OPENVINO_ASSERT
. Thank you.
@@ -763,8 +763,7 @@ void program::cleanup() { | |||
} | |||
} | |||
|
|||
if (_engine.configuration().kernels_cache_path.empty()) | |||
_kernels_cache->reset(); | |||
_kernels_cache->reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
Reverted. Thank you.
@@ -20,7 +20,7 @@ if(ENABLE_ONEDNN_FOR_GPU) | |||
set(ONEDNN_BUILD_DIR "${CMAKE_CURRENT_BINARY_DIR}/onednn_gpu_build/") | |||
set(ONEDNN_INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/onednn_gpu_install/") | |||
set(ONEDNN_PREFIX_DIR "${CMAKE_CURRENT_BINARY_DIR}/onednn_gpu_root") | |||
execute_process(COMMAND git apply --verbose ../onednn_gpu.patch | |||
execute_process(COMMAND git apply ../onednn_gpu.patch OUTPUT_QUIET ERROR_QUIET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
That error message occurs when trying to patch code that has already been patched again. I updated not to be displayed to prevent confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even first cmake run contains this error
1ee8426
to
abc9275
Compare
@@ -47,6 +46,9 @@ class CompiledModel : public InferenceEngine::ExecutableNetworkThreadSafeDefault | |||
Config m_config; | |||
InferenceEngine::ITaskExecutor::Ptr m_taskExecutor; | |||
InferenceEngine::ITaskExecutor::Ptr m_waitExecutor; | |||
|
|||
private: | |||
bool is_serializable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I changed its visibility and name regarding to the coding style. Thank you.
_sizes[1] = _tmp_sizes[1]; | ||
} | ||
buffer << _sizes; | ||
buffer << _layout.get_partial_shape(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
Updated to serialize partial_shape instead of tensor. Thank you.
@@ -474,7 +474,7 @@ void primitive_inst::rebuild_exec_deps( | |||
break; | |||
} | |||
} | |||
OPENVINO_ASSERT(found, _exec_dep_ids[i], "not found in _exec_order"); | |||
OPENVINO_ASSERT(found, _exec_dep_ids[i], "not found in primitives while rebuilding _exec_deps"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I updated error messages here and other places. Thank you.
std::vector<uint8_t> _buf; | ||
_buf.resize(data_size); | ||
ib >> cldnn::make_data(_buf.data(), data_size); | ||
_outputs[0]->copy_from(get_network().get_stream(), _buf.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
Updated to use std::vector<uint8_t> instead of new/delete. Thank you.
std::cout << "[get_index_in_deps]: not found" << std::endl; | ||
|
||
return (idx == _deps.size()) ? -1 : idx; | ||
IE_THROW() << "[get_index_in_deps]: not found in _deps"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
Changed to throw an exception. Thank you.
@@ -439,8 +439,7 @@ void primitive_inst::set_arguments() { | |||
} | |||
|
|||
void primitive_inst::build_deps() { | |||
if (_node == nullptr) | |||
return; | |||
OPENVINO_ASSERT(_node != nullptr, "_node should not be nullptr for build_deps."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
Updated to throw an exception when _node is null. Thank you.
ib >> kernels_cache; | ||
|
||
int num_data_nodes; | ||
ib >> num_data_nodes; | ||
|
||
_memory_pool->clear_pool_for_network(net_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I removed it. Thank you.
8392485
to
7b24290
Compare
@@ -534,7 +540,7 @@ void kernels_cache::save(BinaryOutputBuffer& ob) const { | |||
} | |||
|
|||
void kernels_cache::load(BinaryInputBuffer& ib) { | |||
OPENVINO_ASSERT(_engine.type() == engine_types::ocl, "[GPU] not supported engine type"); | |||
OPENVINO_ASSERT(_engine.type() == engine_types::ocl, "Not supported engine type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep [GPU]
tag for error messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a prefix '[GPU]' to error messages. Thank you.
BIND_BINARY_BUFFER_WITH_TYPE(cldnn::ocl::dft_impl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please setup your ide to insert new line in eof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated them. Thank you.
@@ -20,7 +20,7 @@ if(ENABLE_ONEDNN_FOR_GPU) | |||
set(ONEDNN_BUILD_DIR "${CMAKE_CURRENT_BINARY_DIR}/onednn_gpu_build/") | |||
set(ONEDNN_INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/onednn_gpu_install/") | |||
set(ONEDNN_PREFIX_DIR "${CMAKE_CURRENT_BINARY_DIR}/onednn_gpu_root") | |||
execute_process(COMMAND git apply --verbose ../onednn_gpu.patch | |||
execute_process(COMMAND git apply ../onednn_gpu.patch OUTPUT_QUIET ERROR_QUIET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even first cmake run contains this error
@@ -213,6 +213,30 @@ struct primitive_info { | |||
CLDNN_DEFINE_TYPE_ID(PType) \ | |||
CLDNN_DEFINE_TYPE_STRING(PType) | |||
|
|||
#define CLDNN_DEFINE_PRIMITIVE_TYPE_ID(PType) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use INTEL_GPU
or GPU
prefix intead of CLDNN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it as GPU_DEFINE_PRIMITIVE_TYPE_ID
. Thank you.
} | ||
|
||
private: | ||
std::unordered_map<std::string, cldnn::primitive_type_id> map; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make this map a static field of primitive_type and insert type_id in primitive_type_base c-tor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the point to insert type_id is moved to c-tor, primitives that have not yet been created will not exist in the map. Then, when deserializing, the type_id cannot be obtained by type name, which causes a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
primitive_type objects are static, so all primitives are supposed to be initialized on app startup, aren't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2c6ecac
to
0538a96
Compare
@@ -333,25 +337,19 @@ network::network(cldnn::BinaryInputBuffer& ib, stream::ptr stream, engine& engin | |||
, _internal(false) | |||
, _is_primary_stream(false) | |||
, _reset_arguments(true) { | |||
net_id += 1; | |||
net_id = get_new_net_id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I added a new function get_new_net_id()
to emit an unique id, and applied it to network ctors. Thank you.
// Cache blob format: | ||
// [ ConstInputsDataMap / ConstOutputsDataMap ] | ||
// [ ov::Node::Input/ ov::Node::Output ] | ||
// [ ov::intel_gpu::Graph ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I added cache blob descriptions here, Graph, network, primitive_inst, typed_primitive_impl_ocl and typed_primitive_onednn_impl. Thank you.
1962d76
to
adb8080
Compare
void mutable_data_inst::save(cldnn::BinaryOutputBuffer& ob) const { | ||
parent::save(ob); | ||
|
||
if (!_mem_allocated) { | ||
for (size_t dep_idx = 0; dep_idx < _deps.size(); ++dep_idx) { | ||
for (size_t m_idx = 0; m_idx < _deps[dep_idx]->_deps.size(); ++m_idx) { | ||
if (get_network().get_engine().is_the_same_buffer(*_outputs[0], *_deps[dep_idx]->_deps[m_idx]->_outputs[0])) { | ||
ob << true << dep_idx << m_idx; | ||
return; | ||
} | ||
} | ||
} | ||
} | ||
ob << false; | ||
} | ||
|
||
void mutable_data_inst::load(cldnn::BinaryInputBuffer& ib) { | ||
parent::load(ib); | ||
|
||
bool from_dep; | ||
ib >> from_dep; | ||
if (from_dep && !_mem_allocated) { | ||
size_t dep_idx, m_idx; | ||
ib >> dep_idx >> m_idx; | ||
|
||
auto prev_node = get_network().get_primitive(_dep_ids[dep_idx]); | ||
_outputs[0] = get_network().get_primitive(prev_node->_dep_ids[m_idx])->output_memory_ptr(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I removed this logic as you reviewed. Thank you.
@@ -968,6 +969,17 @@ Parameter Plugin::GetMetric(const std::string& name, const std::map<std::string, | |||
} else if (name == ov::caching_properties) { | |||
std::vector<ov::PropertyName> cachingProperties; | |||
return decltype(ov::caching_properties)::value_type(cachingProperties); | |||
} else if (name == ov::device::architecture) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I added ov::device::architecture
in supported properties. Thank you.
// [ memory dependency information ] | ||
// [ execution dependency information ] | ||
// [ intermediate memory information ] | ||
void primitive_inst::save(cldnn::BinaryOutputBuffer& ob) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13801 (comment)
I overrided save and load methods for data/mutable data, and removed a branch. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
…13986) * moved serialization include path * quiet onednn-gpu patching * save and load kernels in _impls * changed to use OPENVINO_ASSERT * fix errata * updated to follow OpenVINO naming convention * updated error messages * binary buffer by vector<uint8_t> * partial_shape serialization * removed object_type * added a new storage class for primitive_type_string and id * updated to throw an exception when _node is null in build_deps(). * removed redundant memory_pool clearing * added a new net_id creator * newline at eof * updated CLDNN with GPU * added cache blob descriptions * updated output allocation logic in serialization * added ov::device::architecture in supported properties * overrided save and load in data_inst and mutable_data_inst * removed save and load functions in mutable_data * baseline for serialization unit tests * added serialization unit tests * added serialization unit tests * updated not to execute build_deps when deserialized * make_data without namespace * updated to use default layout c-tor * updated get_unique_net_id() * updated get_type_id() to a pure virtual method * updated ov::caching_properties * added [GPU] tags * updated network c-tor * updated unit tests
Details:
Tickets: