Repository navigation
fix(hipblaslt): use portable formatting for 64-bit values to fix warnings - #12326
Conversation
Use stream output for templated matching keys and PRId64 for the grouped GEMM benchmark index. Avoid assuming that long matches the key or int64_t type on Windows. Comparing the saved September 15 local Windows build log with the September 18 hipBLASLt+dist build, format warnings fall from 536 to 0 and total warnings from 562 to 26. The remaining diagnostics are 18 deprecations, 7 C++17 extensions, and 1 potentially evaluated expression. The latest build completed successfully; git diff --cached --check passed. Generated with Codex
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12326 +/- ##
===========================================
+ Coverage 70.78% 70.94% +0.16%
===========================================
Files 2842 2844 +2
Lines 465020 466180 +1160
Branches 68499 68770 +271
===========================================
+ Hits 329152 330715 +1563
+ Misses 112174 111583 -591
- Partials 23694 23882 +188
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
tony-davis
left a comment
There was a problem hiding this comment.
Approving. Nice cleanup, and I appreciate that you linked both logs instead of just asserting the improvement. I pulled them down and counted myself: -Wformat goes 536 to 0, total warnings 562 to 26, and both builds completed with the same 2537 kernels, so it's a real apples-to-apples comparison.
I also checked the types on both sides. Key resolves to std::array<int64_t, N> (via the mappingKey<std::array<int64_t, ...>> dispatch in Serialization/MatchingLibrary.hpp), and in the client ldd and stride_d are std::vector<int64_t>, so PRId64 is right there. Adding <iostream> also closes a latent gap, since the file was already streaming std::cout around line 1060 without including it.
One non-blocking note, take it or leave it. The comparator lambdas at lines 1105, 1113, and 1121 still take long const M / long const N / long const B, and the values passed in are int64_t (e.g. std::min(key[0], (table.end() - 1)->key[0])). So on Windows those get truncated to 32 bits before the comparison, which is the same assumption this PR is removing from the format strings. You'd need a dimension above 2^31 to actually notice, so it's latent and I don't think it should hold the PR. But it's a one-word change to int64_t on lines you're already touching, so I'd probably just grab it while you're in there. Upstream Tensile has the same thing, so there's no parity reason to keep it.
Tiny thing: the Technical Details section says PPRId64 where it means PRId64.
Thanks, fixed (that's what I get for typing descriptions by hand 😅) |
I'd prefer to fix that across both/all code ( |
fix(hipblaslt): use portable formatting for 64-bit values to fix warnings (#12326) ## Motivation This fixes ~500 warnings when building on Windows (see ROCm/TheRock#47), saving 1-2MB of log size per GPU family build. For example: ``` 3.4 In file included from C:/home/runner/_work/TheRock/TheRock/rocm-libraries/projects/hipblaslt/tensilelite/src/msgpack/MessagePack.cpp:27: 3.4 In file included from C:/home/runner/_work/TheRock/TheRock/rocm-libraries/projects/hipblaslt/tensilelite/include\Tensile/msgpack/MessagePack.hpp:31: 3.4 In file included from C:/home/runner/_work/TheRock/TheRock/rocm-libraries/projects/hipblaslt/tensilelite/include\Tensile/ContractionLibrary.hpp:34: 3.4 In file included from C:/home/runner/_work/TheRock/TheRock/rocm-libraries/projects/hipblaslt/tensilelite/include\Tensile/GranularitySelectionLibrary.hpp:38: 3.4 C:/home/runner/_work/TheRock/TheRock/rocm-libraries/projects/hipblaslt/tensilelite/include\Tensile/PropertyMatching.hpp:1106:67: warning: format specifies type 'long' but the argument has type 'long long' [-Wformat] 3.4 1106 | printf("[ %ld ,%ld,%ld,%ld]\n", e.key[0], e.key[1], e.key[2], e.key[3]); 3.4 | ~~~ ^~~~~~~~ 3.4 | %lld 3.4 C:/home/runner/_work/TheRock/TheRock/rocm-libraries/projects/hipblaslt/tensilelite/include\Tensile/PropertyMatching.hpp:1106:77: warning: format specifies type 'long' but the argument has type 'long long' [-Wformat] 3.4 1106 | printf("[ %ld ,%ld,%ld,%ld]\n", e.key[0], e.key[1], e.key[2], e.key[3]); 3.4 | ~~~ ^~~~~~~~ 3.4 | %lld ... 3.4 C:/home/runner/_work/TheRock/TheRock/rocm-libraries/projects/hipblaslt/tensilelite/src/msgpack/MessagePack.cpp:413:9: note: in instantiation of function template specialization 'TensileLite::MessagePackLoadLibraryFile<TensileLite::ContractionProblemGemm, TensileLite::ContractionSolution>' requested here 3.4 413 | MessagePackLoadLibraryFile<ContractionProblemGemm, ContractionSolution>( 3.4 | ^ ``` ## Technical Details The `%ld` format specifier expects `long`, while the values are `int64_t`. * On 64-bit Linux, `long` is typically 64 bits * On 64-bit Windows, `long` is 32 bits while `int64_t` is always 64 bits (`long long`) See https://en.cppreference.com/cpp/types/integer#Format_macro_constants, `PRId64` is recommended for `int64_t`. > [!NOTE] > The `tensilelite/include/Tensile/PropertyMatching.hpp` fix was already applied to _Tensile_ in ROCm/Tensile#1623 / ROCm/rocm-libraries@37278db, including switching from `printf` to `std::cout`. > https://github.com/ROCm/rocm-libraries/blob/ab0a5742e4480ade753a606fab6d6945fd2c0a51/shared/tensile/Tensile/Source/lib/include/Tensile/PropertyMatching.hpp#L698-L701 ## Test Plan Check that CI logs no longer contain these warnings. Run details | Logs link | file size -- | -- | --: baseline | [35375442872-windows/...hipBLASLt_build.log](https://therock-ci-artifacts-external.s3.amazonaws.com/ROCm-rocm-libraries/35375442872-windows/logs/math-libs/gfx1151/hipBLASLt_build.log) | 1247KB this PR | [35397683840-windows/...hipBLASLt_build.log](https://therock-ci-artifacts-external.s3.amazonaws.com/ROCm-rocm-libraries/35397683840-windows/logs/math-libs/gfx1151/hipBLASLt_build.log) | 87KB ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests.
TheRock Submodule Bump ActivityNewest first
|
Motivation
This fixes ~500 warnings when building on Windows (see ROCm/TheRock#47), saving 1-2MB of log size per GPU family build.
For example:
Technical Details
The
%ldformat specifier expectslong, while the values areint64_t.longis typically 64 bitslongis 32 bits whileint64_tis always 64 bits (long long)See https://en.cppreference.com/cpp/types/integer#Format_macro_constants,
PRId64is recommended forint64_t.Note
The
tensilelite/include/Tensile/PropertyMatching.hppfix was already applied to Tensile in ROCm/Tensile#1623 / 37278db, including switching fromprintftostd::cout.rocm-libraries/shared/tensile/Tensile/Source/lib/include/Tensile/PropertyMatching.hpp
Lines 698 to 701 in ab0a574
Test Plan
Check that CI logs no longer contain these warnings.
Submission Checklist