Conversation
WalkthroughThe changes introduce support for the ROCM execution provider in the ONNX runtime integration. This includes adding a new enumerator to the provider enum, updating the string-to-provider mapping to recognize "rocm", and handling ROCM-specific logic in session options configuration, including error handling if ROCM support is not enabled. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sherpa-onnx/csrc/session.cc (1)
181-193: LGTM with minor error message improvement.The ROCm provider implementation follows the established pattern used by other providers (CUDA, etc.) and correctly handles:
- Provider availability checking
- Device ID configuration with fallback to default
- Session options configuration
- Error handling and CPU fallback
Consider improving the error message clarity:
- "Please compile with ort enable ROCM EP. Available " + "Please compile with ROCm execution provider support enabled. Available "
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sherpa-onnx/csrc/provider.cc(1 hunks)sherpa-onnx/csrc/provider.h(1 hunks)sherpa-onnx/csrc/session.cc(1 hunks)
🔇 Additional comments (2)
sherpa-onnx/csrc/provider.cc (1)
31-32: Update enum reference after fixing naming inconsistency.The logic and implementation pattern are correct. However, this code references
Provider::KROCMwhich should be updated toProvider::kROCMafter fixing the naming inconsistency in the header file.After updating the enum name in provider.h, apply this diff:
- } else if (s == "rocm") { - return Provider::KROCM; + } else if (s == "rocm") { + return Provider::kROCM;sherpa-onnx/csrc/session.cc (1)
180-180: Update enum reference after fixing naming inconsistency.This case statement references
Provider::KROCMwhich should be updated toProvider::kROCMafter fixing the naming inconsistency in the header file.After updating the enum name in provider.h, apply this diff:
- case Provider::KROCM: { + case Provider::kROCM: {
sherpa-onnx/csrc/session.cc
Outdated
| "ROCMExecutionProvider") != available_providers.end()) { | ||
| OrtROCMProviderOptions options; | ||
| options.device_id = | ||
| provider_config != nullptr ? provider_config->device : 0; |
There was a problem hiding this comment.
| provider_config != nullptr ? provider_config->device : 0; | |
| provider_config? provider_config->device : 0; |
| sess_opts.AppendExecutionProvider_ROCM(options); | ||
| } else { | ||
| SHERPA_ONNX_LOGE( | ||
| "Please compile with ort enable ROCM EP. Available " |
There was a problem hiding this comment.
Can you add some doc to describe how to build sherpa-onnx with ROCM support?
Do we need to change the toplevel CMakeLists.txt?
There was a problem hiding this comment.
do not to change the CMaleLists.txt , only need to set env SHERPA_ONNXRUNTIME_INCLUDE_DIR , SHERPA_ONNXRUNTIME_LIB_DIR before use cmake to build .
hygon prebuilt onnxruntime :
https://cancon.hpccube.com:65024/4/main/onnxruntime/DAS1.6
amd prebuilt onnxruntime :
I will built and upload .
There was a problem hiding this comment.
Can you follow
https://github.com/k2-fsa/sherpa-onnx/blob/master/cmake/onnxruntime-win-x64-directml.cmake
to support rocm?
It will simplify how users use rocm.
There was a problem hiding this comment.
yes. but currently , I did not find onnxruntime-rocm on nuget ,conan or vcpkg . I will build a onnxruntime-rocm and update Cmake.
2 participants
add rocm ep for amd & hygon gpu support. tested when hip_version >=5.4 .
test dolphin small on hygon k100-ai dcu , dtk version >= 25.04 ,onnxruntime 1.19.2 .
note: only f32 model runs on hygon dcu . int8 model will fallback to cpu.
Summary by CodeRabbit
New Features
Bug Fixes