Skip to content

Slaurenz therock win stage #1956

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

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

stellaraccident
Copy link
Contributor

No description provided.

@stellaraccident stellaraccident force-pushed the slaurenz_therock_win_stage branch 2 times, most recently from 5e16655 to 804c9d1 Compare April 21, 2025 23:49
stellaraccident and others added 9 commits April 21, 2025 17:07
…onfigure time PATH.

* Makes gen_assembly.sh and compile_code_object.sh scripts fail on error vs silently succeeding (`set -e`). These scripts have a suspect design overall, but making them error on failure is a minimal thing that is needed.
* Do not hard-code /opt/rocm tooling paths in these shell scripts. Instead use an explicit `ROCM_PATH` if specified, falling back to `PATH` resolution like the rest of the project. This is still looser than I would like (I would prefer explicit configuration of tools), but this at least makes things work. We should never have hard-coded /opt/rocm paths in scripts.
* Wraps build time tool invocation in `cmake -E env PATH=...` to propagate the configure time PATH to build time. Otherwise, this project cannot build with separate configure/build environments, which is a standard practice for assembling super-projects.
* Escapes the PATH environment variable so it is suitable to pass to COMMANDs on either platform.
* On Windows, also validates that `bash` is a usable native bash vs the WSL system bash that MSFT installs by default and is incompatible with native tooling.

In general, hipBLASLt is very sloppy in terms of how it finds its tools. I resisted the urge to attempt to clean that up because the refactoring would need to be quite extensive. This is a minimal set of changes to at least make what was written correct.

This will have a subtle change that if the project was not build with an explicit ROCM_PATH and was somehow relying on falling back to /opt/rocm implicitly, that will now fail. As it should.
Use the config files provided by upstream instead of a custom finder.
So far it is assumed that NETLIB BLAS and LAPACK are avilable without
checking. If they are nore, the build fails when trying to link.
Switches to `find_package()` therefore which also enables using
OpenBLAS.
When using setuptools>=71.0.0 and packaging<22.0, setuptools may fail with `TypeError: canonicalize_version() got an unexpected keyword argument 'strip_trailing_zero'`.
* Pinning clocks was already conditioned on this, so the cleanup logic should also be.
* This is a legacy variable that is not recommended for modern use and is only available via the GTest find module, not the actual config provided by the package.
* Recommended usage is just to depend on the libraries.
* Also removes GTEST_INCLUDE_DIRS, since that is implicit in depending on the library.
This patch is not to be landed but just makes some coarse changes to the legacy CMake files to let them configure/build at all on Windows. Actual Windows support will target the new CMake build files being written now.
@stellaraccident stellaraccident force-pushed the slaurenz_therock_win_stage branch from 804c9d1 to b4f4cbc Compare April 22, 2025 00:07
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.

3 participants