Fix reorder ops in cpu implementation with cl_mem #28286
Conversation
| @@ -56,7 +56,7 @@ struct reorder_impl : public typed_primitive_impl<reorder> { | |||
| auto output_mem_ptr = instance.output_memory_ptr(); | |||
|
|
|||
| cldnn::mem_lock<uint8_t, mem_lock_type::read> input_lock(input_mem_ptr, stream); | |||
| cldnn::mem_lock<uint8_t, mem_lock_type::read> output_lock(output_mem_ptr, stream); | |||
| cldnn::mem_lock<uint8_t, mem_lock_type::read_write> output_lock(output_mem_ptr, stream); | |||
There was a problem hiding this comment.
output memory will be updated as the reason solved by this PR, could you descript the detail reason?
There was a problem hiding this comment.
who allocate cl_mem output for this node? and why cpu impl is choosen?
btw, this line will break if output is usm_device, it's unsafe to directly change to read_write here
There was a problem hiding this comment.
Sure, River, I will add some test case to explain this
There was a problem hiding this comment.
usm_device
If it is usm_device, it will do copying operation by mem_lock itself, right? But we need logic to write back to usm_device memory if needed.
There was a problem hiding this comment.
@WeldonWangwang, could you please update access modifiers for the other CPU impls as well?
There was a problem hiding this comment.
@sshlyapn Sure, will add it.
Co-authored-by: River Li <[email protected]>
| @@ -39,6 +39,9 @@ struct gpu_buffer : public lockable_gpu_mem, public memory { | |||
| assert(0 == _lock_count); | |||
| return _buffer; | |||
| } | |||
| void* buffer_ptr() const override { | |||
There was a problem hiding this comment.
I believe it's not needed?
There was a problem hiding this comment.
Hi Sergey, it's not needed, this API just used in debug messages here:
The ptr would be 0 without this API, so I added it.
such as:
GPU_Debug: sync_infer_request.cpp:989:prepare_output: result:Result_51154 with index 9 prepare output: 0
| @@ -56,7 +56,7 @@ struct reorder_impl : public typed_primitive_impl<reorder> { | |||
| auto output_mem_ptr = instance.output_memory_ptr(); | |||
|
|
|||
| cldnn::mem_lock<uint8_t, mem_lock_type::read> input_lock(input_mem_ptr, stream); | |||
| cldnn::mem_lock<uint8_t, mem_lock_type::read> output_lock(output_mem_ptr, stream); | |||
| cldnn::mem_lock<uint8_t, mem_lock_type::read_write> output_lock(output_mem_ptr, stream); | |||
There was a problem hiding this comment.
@WeldonWangwang, could you please update access modifiers for the other CPU impls as well?
Details:
When use HETERO:GPU.0,GPU.1 pipeline parallel split the llama_v2 model, there is a
__module.model.layers.0.self_attn/prim::ListConstruct_3node is marked in shape_of subgraph, and it's result will pass to next device, thus this node and it's result node can only get CPU implementations.In inference stage, when prepare outputs for the Result_44438, B580 dGPU will only use cl_mem in
openvino/src/plugins/intel_gpu/src/plugin/sync_infer_request.cpp
Line 548 in d757efd
So, we have two solutions:
Tickets: