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

[UR] Enable hwloc in UMF linux build #15261

Open
wants to merge 1 commit into
base: sycl
Choose a base branch
from

Conversation

PatKamin
Copy link

@PatKamin PatKamin commented Sep 3, 2024

Update UR to statically link UMF with hwloc on Linux.
This will enable hwloc support in UMF on Linux.

Also, always use the v2.9.3 hwloc version for hwloc builds.

@PatKamin PatKamin marked this pull request as ready for review September 13, 2024 13:35
@PatKamin PatKamin requested a review from a team as a code owner September 13, 2024 13:35
@PatKamin PatKamin force-pushed the enable-hwloc-on-linux branch from d8a76ff to 94c1a73 Compare December 6, 2024 08:47
@PatKamin PatKamin force-pushed the enable-hwloc-on-linux branch from 7aa9690 to dcc6761 Compare December 9, 2024 12:11
@PatKamin PatKamin force-pushed the enable-hwloc-on-linux branch from dcc6761 to 2891f1e Compare December 9, 2024 12:16
@PatKamin PatKamin force-pushed the enable-hwloc-on-linux branch from 2891f1e to c647376 Compare December 9, 2024 12:28
@PatKamin PatKamin marked this pull request as draft December 9, 2024 13:58
@PatKamin
Copy link
Author

PatKamin commented Dec 9, 2024

Drafted until the UMF fix is pulled down to llvm via UMF update in UR.

@PatKamin PatKamin force-pushed the enable-hwloc-on-linux branch from c647376 to c6e5f06 Compare December 12, 2024 09:01
@PatKamin PatKamin marked this pull request as ready for review December 12, 2024 09:01
@PatKamin
Copy link
Author

Drafted until the UMF fix is pulled down to llvm via UMF update in UR.

Fix is now pulled down, ready for review.

@igchor
Copy link
Member

igchor commented Dec 30, 2024

You should probably also update https://github.com/intel/llvm/blob/sycl/sycl/doc/GetStartedGuide.md (remove hwloc as a dependency that needs to be installed by the user)

@PatKamin PatKamin force-pushed the enable-hwloc-on-linux branch from c6e5f06 to f1fa58b Compare December 31, 2024 11:36
@PatKamin PatKamin requested a review from a team as a code owner December 31, 2024 11:36
@PatKamin
Copy link
Author

You should probably also update https://github.com/intel/llvm/blob/sycl/sycl/doc/GetStartedGuide.md (remove hwloc as a dependency that needs to be installed by the user)

Right, now it will automatically be downloaded - deleted user requirement.

Comment on lines -47 to -48
* `hwloc` version 2.3 or later (Linux only)
* libhwloc-dev or hwloc-devel package on linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update UR to statically link hwloc v2.9.3 both on linux and Windows.

If I get the change right, the dependency on hwloc still exists. The change removed the runtime dependency, but not the compile time dependency. Am I right?

This document describes the environment for DPC++ compiler developers, not DPC++ compiler users.

Your change seems to raise the version requirement. Please, update the hwloc version to 2.9.3.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hwloc library will always be fetched from sources. Regardless of hwloc-dev packages user has installed, the fetched version will be linked with the UMF library. That's why I remove hwloc from this dependencies list, as the user-installed hwloc-dev package will not be used by UMF.

@bader
Copy link
Contributor

bader commented Dec 31, 2024

You should probably also update https://github.com/intel/llvm/blob/sycl/sycl/doc/GetStartedGuide.md (remove hwloc as a dependency that needs to be installed by the user)

Right, now it will automatically be downloaded - deleted user requirement.

If hwloc dependencies will be automatically resolved by CMake, please, remove installation of these dependencies from CI scripts. Specifically, from

and
sudo apt-get install -y graphviz ssh ninja-build libhwloc-dev
.

@PatKamin PatKamin force-pushed the enable-hwloc-on-linux branch from f1fa58b to d54b9d2 Compare January 2, 2025 08:48
@PatKamin PatKamin requested a review from a team as a code owner January 2, 2025 08:48
@PatKamin
Copy link
Author

PatKamin commented Jan 2, 2025

You should probably also update https://github.com/intel/llvm/blob/sycl/sycl/doc/GetStartedGuide.md (remove hwloc as a dependency that needs to be installed by the user)

Right, now it will automatically be downloaded - deleted user requirement.

If hwloc dependencies will be automatically resolved by CMake, please, remove installation of these dependencies from CI scripts. Specifically, from

and

sudo apt-get install -y graphviz ssh ninja-build libhwloc-dev

.

Thanks, done

Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need brew install hwloc on MacOS build CI workflow: https://github.com/intel/llvm/blob/sycl/.github/workflows/sycl-macos-build-and-test.yml#L33 ?

Update UR to statically link hwloc v2.9.3 both on linux and Windows.

I recall installing hwloc on Windows CI runners. Now that we are fetching hwloc from sources and statically linking it, am I good to remove hwloc installations on Windows CI runners once this PR gets merged?

Also, can you please update the PR title? - at the first glace it looked like the change is for Linux only. But the description says that the change is for both Windows and Linux.

@PatKamin
Copy link
Author

PatKamin commented Jan 3, 2025

Do we still need brew install hwloc on MacOS build CI workflow: https://github.com/intel/llvm/blob/sycl/.github/workflows/sycl-macos-build-and-test.yml#L33 ?

I've removed it now, it shouldn't be required anymore.

I recall installing hwloc on Windows CI runners. Now that we are fetching hwloc from sources and statically linking it, am I good to remove hwloc installations on Windows CI runners once this PR gets merged?

openmp still needs hwloc, so this can be done with a caution on Windows CI runners that are not used for openmp tests, I think.

Also, can you please update the PR title? - at the first glace it looked like the change is for Linux only. But the description says that the change is for both Windows and Linux.

I've updated the misleading commit message. hwloc already is linked to UMF statically on Windows even without this change.

Copy link
Contributor

@aarongreig aarongreig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UR LGTM

Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI changes LGTM.

Update UR to statically link UMF with hwloc on Linux.
This will enable hwloc support in UMF on Linux.

Also, always use the v2.9.3 hwloc version for hwloc builds.
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.

5 participants