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

bump up version for rccl for rocm-4.1.0 release #22762

Merged
merged 3 commits into from
Apr 7, 2021

Conversation

srekolam
Copy link
Contributor

@srekolam srekolam commented Apr 2, 2021

@haampie , while updating the version for rccl package for rocm-4.1.0 i see an error as below. I see similar error with respect to few other packages for rocm-4.1.0 eg -hipcub, rocprim,rocrand etc..
Any thoughts ?
rccl_4.1.0_spack-build-out.txt

@srekolam srekolam requested a review from haampie April 2, 2021 22:03
@haampie
Copy link
Member

haampie commented Apr 3, 2021

Can you check this with your team?

Between 4.0.0 and 4.1.0 it seems HIP_LIB_PATH has changed from hip-4.0.0-.../lib to hip-rocclr-4.1.0-.../lib

@srekolam srekolam requested a review from tldahlgren April 5, 2021 22:54
@srekolam srekolam added the AMD label Apr 5, 2021
@srekolam
Copy link
Contributor Author

srekolam commented Apr 5, 2021

@haampie , i have updated versions for rocm-4.1.0 as well as fixed the issue mentioned with hip package - #22768

@@ -58,6 +58,7 @@ class Hip(CMakePackage):
# See https://github.com/ROCm-Developer-Tools/HIP/pull/2218
patch('0003-Improve-compilation-without-git-repo.3.7.0.patch', when='@3.7.0:3.9.0')
patch('0003-Improve-compilation-without-git-repo.3.10.0.patch', when='@3.10.0:4.0.0')
patch('0003-Improve-compilation-without-git-repo.4.1.0.patch', when='@4.1.0')
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason this patch is not getting merged upstream?

Copy link
Member

Choose a reason for hiding this comment

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

ROCm/HIP#2218 I tried. Let me bump that thread again.

Copy link
Member

Choose a reason for hiding this comment

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

@haampie I saw that, and I think the patch is appropriate. I was wondering though why it is not accepted, since it seems we have to maintain slightly modified versions of it for different version ranges of the package 🙁

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... another way to fix it is to use git instead of tarballs, but I think they should just support non-git builds.

@haampie
Copy link
Member

haampie commented Apr 6, 2021

@srekolam can you maybe fix this wrong -L flag issue by setting HIP_LIB_PATH as (yet) another environment variable to hip's lib folder?

The HIP_ROCCLR_... var should not be set because of this in HIP:

if (defined $HIP_ROCCLR_HOME) {
    $HIP_INFO_PATH= "$HIP_ROCCLR_HOME/lib/.hipInfo";
} else {
    $HIP_INFO_PATH= "$HIP_PATH/lib/.hipInfo"; # use actual file
}

there is no .hipInfo in the rocclr/lib folder.

@srekolam
Copy link
Contributor Author

srekolam commented Apr 7, 2021

can i get PR approved. did not see any changes to be made..

@haampie
Copy link
Member

haampie commented Apr 7, 2021

Can you set the HIP_LIB_PATH env variable to <hip prefix>/lib? That way we would not hit these branches:

https://github.com/ROCm-Developer-Tools/HIP/blob/rocm-3.9.0/bin/hipcc#L162
https://github.com/ROCm-Developer-Tools/HIP/blob/rocm-4.1.0/bin/hipcc#L125

@srekolam
Copy link
Contributor Author

srekolam commented Apr 7, 2021

@haampie , i think it is not required .. i looked at hip@4.0.0 and before and i see it existed before similar to what is in hip@4.1.0. Any way if required, i will need to check with the HIP team and propose to bring it in a new pr.

@haampie
Copy link
Member

haampie commented Apr 7, 2021

Okay, let's do that seperately then

@haampie haampie merged commit 56418ba into spack:develop Apr 7, 2021
@srekolam srekolam mentioned this pull request Apr 7, 2021
RikkiButler20 pushed a commit to RikkiButler20/spack that referenced this pull request Jun 9, 2021
sethrj pushed a commit to sethrj/spack that referenced this pull request Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants