Compatibility fixes for building on Windows. #1591
Conversation
ScottTodd
requested review from
amcamd,
TorreZuk,
mahmoodw,
daineAMD,
bragadeesh,
NaveenElumalaiAMD,
rkamd,
yoichiyoshida and
babakpst
as code owners
There was a problem hiding this comment.
Our windows build and tests passed but of course with AOCL. Can you mention the version of OpenBLAS you are using. As we use AOCL in our CI so can you work on full quality control testing with OpenBLAS on windows. Did you run all tests? I made a tech debt ticket but if you have CI nodes testing with OpenBLAS it may be better if you can work on it.
|
PS - as our windows build didn't need the include changes curious how yours diverges in build flags or compiler. Could be a LEAN variation... but can be figure out. |
We are using OpenBLAS v0.3.29 in TheRock at the moment. That is latest release on GitHub, from 2025-01-12: https://github.com/OpenMathLib/OpenBLAS/releases. That version is pinned here: https://github.com/ROCm/TheRock/blob/9b7ef5e13ac1f4f7d2fea75639785e08a464bc43/third-party/host-blas/CMakeLists.txt#L10-L19 and made available to
I ran some tests on my dev machine (AMD Radeon Pro W7900, gfx1200): ROCm/TheRock#482 (comment). Initial results were promising, with only a few failures that I have not triaged yet like this: I stopped the test process after ~10 minutes to switch tasks. We're still bootstrapping projects and have not yet onboarded and validated all tests or enabled them in continuous integration workflows on dedicated test machines. That will come once enough of the projects (and all their transitive deps) configure/build as expected.
Yeah that's strange to me too. I got the hint to switch the include order for |
| #ifdef WIN32 | ||
| // Must include windows.h before dependent headers. | ||
| // Specifically this must be before client_utility.hpp uses `#pragma GCC poison` | ||
| // to poison 'ctime' and 'abort', as those are used in standard library headers. | ||
| #include <windows.h> |
There was a problem hiding this comment.
If no Cryptography, DDE, RPC, Shell, and Windows sockets are required, you should prepend #include <Windows.h> with #define WIN32_LEAN_AND_MEAN as such:
#idefine WIN32_LEAN_AND_MEAN
#include <Windows.h>The correct case of the Windows.h is with the capital letter (for MSFT)
There are more flags to minimize the footprint. Read it more here: https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers
But usually WIN32_LEAN_AND_MEAN is enough.
There was a problem hiding this comment.
The LEAN define is added in our toolchain-windows.cmake where used with the original fail fast design pattern. This is a new windows build pipeline which may not use that, I will need to confirm that sometime, but for now if both builds succeed this is fine to merge.
There was a problem hiding this comment.
TheRock currently sets the same defines for each ROCm sub-project with a common toolchain file constructed here: https://github.com/ROCm/TheRock/blob/b5412338880fed74976b2f4408e0e06cbccc683f/cmake/therock_subproject.cmake#L1111-L1119 . I did not need to add the WIN32_LEAN_AND_MEAN define for build correctness, but agree that it could be beneficial to add. Definitely worth doing a scrub through and setting more build size and performance flags, but might need to customize defines/flags per sub-project as in the current project-specific toolchain files.
There was a problem hiding this comment.
Well that explains the error you see vs. what we get in our windows CI. It is not for correctness but for fewer dependencies being included. Some component other than rocBLAS may use the dependencies @apwojcik listed but probably not if they copied the rocBLAS toolchain (which was done first).
|
@ScottTodd are you going to merge this or close this PR and adopt WIN32_LEAN_AND_MEAN ? |
I don't have permissions to merge. The OPENBLAS change will be required regardless, but I can test if the |
|
@ScottTodd I can merge now if you want, as okay to merge support for no define set |
|
Just got a successful build of rocBLAS with the |
|
So yes, please merge when you like :) |
FYI that went well: ROCm/TheRock#607. Looks like all subprojects are compatible with |
Related to #36 and #482. See discussion over at ROCm/rocBLAS#1591, where adding the `WIN32_LEAN_AND_MEAN` define was recommended as a way to resolve build errors instead of reordering includes. That define is generally recommended for Windows builds, so long as "less common APIs" are unused: https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers#faster-builds-with-smaller-header-files > You can reduce the size of the Windows header files by excluding some of the less common API declarations as follows: > * Define WIN32_LEAN_AND_MEAN to exclude APIs such as Cryptography, DDE, RPC, Shell, and Windows Sockets. > > `#define WIN32_LEAN_AND_MEAN`
resolves ROCm/TheRock#482
Summary of proposed changes:
OpenBLASpackage.#include <windows.h>.