Skip to content

Update CK and fix performance issue on dev machine - #13531

Merged
cloudhan merged 6 commits into
mainfrom
guangyunhan/update-ck-fix-opt
Nov 3, 2022
Merged

cloudhan merged 6 commits into
mainfrom
guangyunhan/update-ck-fix-opt

Conversation

@cloudhan

@cloudhan cloudhan commented Nov 1, 2022

Copy link
Copy Markdown
Contributor

Reland #13493 with problem solved, also updated.

  1. Update CK to its latest develop branch
  2. -mllvm -amdgpu-early-inline-all=true is critical to CK's performance, ensure it is properly configured.
    • The flags are propagated from target hip-lang::device's INTERFACE_COMPILE_OPTIONS, we must not manually add the flags.
    • Instead, we must ensure this target is properly configured by checking _CMAKE_HIP_DEVICE_RUNTIME_TARGET is set.

TL,DR:

hip-lang::device sometime will be not be properly configured if our CMAKE_PREFIX_PATH is not configured carefully. In the CI docker, the configuration is in good state, but on dev machine it is not, which then silently result poor performance for kernels. We fixed it in this PR and add a guard to avoid unsuccessful future editing and to prevent convoluted debugging process.

_CMAKE_HIP_DEVICE_RUNTIME_TARGET is shared in /opt/rocm/lib/cmake/hip-lang/hip-lang-config.cmake and it is internal to CMake, the variable name will not be changed in the foreseeable future.

…red.

The flags must be added to all HIP sources, we ensure it by checking hip-lang::device is properly configured.
@cloudhan cloudhan changed the title Guangyunhan/update ck fix opt Update CK and fix performance issue on dev machine Nov 1, 2022
onnxruntime_add_shared_library_module(onnxruntime_providers_rocm ${onnxruntime_providers_rocm_src})

if(NOT MSVC)
target_compile_options(onnxruntime_providers_rocm PRIVATE -Wno-sign-compare -D__HIP_PLATFORM_HCC__=1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you elaborate a bit why this flag and the above ones are not needed anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They are not used anymore. Just clean up the leftover after #13406

…vel.

Note this have no implication on performance.
@cloudhan
cloudhan merged commit 2de883c into main Nov 3, 2022
@cloudhan
cloudhan deleted the guangyunhan/update-ck-fix-opt branch November 3, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants