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.

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

[HIP Package] HIP version from cmake is unreliable #2651

Open
junliume opened this issue Jan 3, 2024 · 6 comments
Open

[HIP Package] HIP version from cmake is unreliable #2651

junliume opened this issue Jan 3, 2024 · 6 comments

Comments

@junliume
Copy link
Collaborator

junliume commented Jan 3, 2024

HIP currently uses GITDATE to cmake package version and not using PATCH version. The following is not reporting correct version as reported by hipcc --version

find_package(hip REQUIRED PATHS /opt/rocm)
message(STATUS "Build with HIP ${hip_VERSION}")

Until this issue is resolved, MIOpen needs to work around to incremental and backward-incompatible hipRTC changes.

Mentioned:

Keep track of code changes necessary when the exact version is fixed:

  • \todo: exact version from which this WA is needed should be updated in release
  • src/kernels/miopen_cstdint.hpp
  • src/kernels/miopen_type_traits.hpp
  • src/comgr.cpp
  • test/handle_test.cpp
@junliume
Copy link
Collaborator Author

junliume commented Jan 5, 2024

@atamazov now the mainline HIP version for minor version is fixed, so the cmake should identify the HIP package version correctly to 6.1. However, the patch version is still not fixed thus not reliable.

Could we modify on top of merged #2644 and #2652, and eliminate the requirement of forced HIP version in cmake? Thanks!

update:
I guess this line would be challenging ...

#if HIP_PACKAGE_VERSION_FLAT >= 6001000000ULL && HIP_PACKAGE_VERSION_FLAT < 6001024000ULL

@atamazov
Copy link
Contributor

atamazov commented Jan 5, 2024

@junliume

Could we modify on top of merged #2644 and #2652, and eliminate the requirement of forced HIP version in cmake?

We do not need to modify anything in MIOpen. If minor version is fixed, then we simply do not need to provide CMake with -DMIOPEN_OVERRIDE_HIP_VERSION_MINOR=... when we build the library.

@atamazov
Copy link
Contributor

atamazov commented Jan 5, 2024

@junliume

update: I guess this line would be challenging ...

#if HIP_PACKAGE_VERSION_FLAT >= 6001000000ULL && HIP_PACKAGE_VERSION_FLAT < 6001024000ULL

If you mean #2652 (comment), then please do not worry, I'll prepare the fix.

@junliume
Copy link
Collaborator Author

junliume commented Jan 5, 2024

@junliume

update: I guess this line would be challenging ...

#if HIP_PACKAGE_VERSION_FLAT >= 6001000000ULL && HIP_PACKAGE_VERSION_FLAT < 6001024000ULL

If you mean #2652 (comment), then please do not worry, I'll prepare the fix.

@atamazov in this case, do we still need to provide to cmake command with -DMIOPEN_OVERRIDE_HIP_VERSION_PATCH=24000? The patch version is still not reliable at the moment.

@atamazov
Copy link
Contributor

atamazov commented Jan 5, 2024

@junliume It depends on HIP version reported by future HIP staging, mainline and releases. And also on possible future functional changes in HIP that could enforce us to make more changes in the library.

But overriding could be allowed for staging and mainline HIP only. The adaptations must ensure that library works correctly out of the box with with all HIP releases, -- that is the implied goal.

@atamazov
Copy link
Contributor

Some changes in the patch version provided by HIP: #2764 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants