ROCm / MIOpen

AMD's Machine Intelligence Library
https://rocm.docs.amd.com/projects/MIOpen/en/latest/
Other
1.08k stars 230 forks source link

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

Open junliume opened 10 months ago

junliume commented 10 months ago

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:

junliume commented 10 months ago

@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 ... https://github.com/ROCm/MIOpen/blob/0fab57973b3e04e9d1d8f2558f8b0030487b7eec/src/kernels/miopen_type_traits.hpp#L79

atamazov commented 10 months ago

@junliume

Could we modify on top of merged https://github.com/ROCm/MIOpen/pull/2644 and https://github.com/ROCm/MIOpen/pull/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 commented 10 months ago

@junliume

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

https://github.com/ROCm/MIOpen/blob/0fab57973b3e04e9d1d8f2558f8b0030487b7eec/src/kernels/miopen_type_traits.hpp#L79

If you mean https://github.com/ROCm/MIOpen/pull/2652#discussion_r1442577220, then please do not worry, I'll prepare the fix.

junliume commented 10 months ago

@junliume

update: I guess this line would be challenging ... https://github.com/ROCm/MIOpen/blob/0fab57973b3e04e9d1d8f2558f8b0030487b7eec/src/kernels/miopen_type_traits.hpp#L79

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 commented 10 months ago

@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 commented 8 months ago

Some changes in the patch version provided by HIP: https://github.com/ROCm/MIOpen/pull/2764#discussion_r1526514836