Skip to content
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.

Sign up for GitHub

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

Fixes SWDEV-483388 #74

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Fixes SWDEV-483388 #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d039d2
Fix SWDEV-483388: mistakenly invoke hipMemcpy when ORT_ENABLE_STREAM=…
xinyazhang Oct 16, 2024
77ecac5
Fix another hipMemcpy
xinyazhang Oct 18, 2024
7c73ec5
Remove MIGRAPHX_STREAM_SYNC guard which does not exists in current ve…
xinyazhang Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion onnxruntime/core/providers/migraphx/gpu_data_transfer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
HIP_CALL_THROW(hipMemcpyAsync(dst_data, src_data, bytes, hipMemcpyDeviceToDevice, static_cast<hipStream_t>(stream.GetHandle())));
} else {
// copy from other CPU memory to GPU, this is blocking
HIP_CALL_THROW(hipMemcpy(dst_data, src_data, bytes, hipMemcpyHostToDevice));
HIP_CALL_THROW(hipMemcpyWithStream(dst_data, src_data, bytes, hipMemcpyHostToDevice, static_cast<hipStream_t>(stream.GetHandle())));

Check warning on line 60 in onnxruntime/core/providers/migraphx/gpu_data_transfer.cc

View workflow job for this annotation

GitHub Actions / cpplint

[cpplint] onnxruntime/core/providers/migraphx/gpu_data_transfer.cc#L60 

 Lines should be <= 120 characters long  [whitespace/line_length] [2]
Raw output 
onnxruntime/core/providers/migraphx/gpu_data_transfer.cc:60:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix lint. Also, I thought we wanted to block in this case, but I suppose this allows only MIGraphX EP to block on stream now since this gpu_data_transfer isn't shared between both EPs.

}
} else if (src_device.Type() == OrtDevice::GPU) {
HIP_CALL_THROW(hipMemcpyAsync(dst_data, src_data, bytes, hipMemcpyDeviceToHost, static_cast<hipStream_t>(stream.GetHandle())));
Expand Down
6 changes: 5 additions & 1 deletion onnxruntime/core/providers/migraphx/migraphx_execution_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,11 @@ Status MIGraphXExecutionProvider::Compile(const std::vector<FusedNodeAndGraph>&
std::vector<int64_t> ort_shape{res_lens.begin(), res_lens.end()};
auto output_tensor = ctx.GetOutput(i, ort_shape.data(), ort_shape.size());
void* output_data = output_tensor.GetTensorMutableRawData();
HIP_CALL_THROW(hipMemcpy(output_data, gpu_res.data(), res_shape.bytes(), hipMemcpyDeviceToDevice));
HIP_CALL_THROW(hipMemcpyWithStream(output_data,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With MIGraphX we perform the run_async as part of the eval() which streams execution. Were you seeing stale data somehow with this memcopy for networks with many outputs? Tried this with Bert and didn't see a change in perf but curious as to why this changed.

gpu_res.data(),
res_shape.bytes(),
hipMemcpyDeviceToDevice,
static_cast<hipStream_t>(rocm_stream)));
}
}
};
Expand Down
Loading