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.

Sign up for GitHub

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

Jump to bottom

Updates to rocm recipes for rocm-5.5.0 and rocm-5.5.1 releases #37910

Merged
merged 24 commits into from
Jul 27, 2023
Merged

Updates to rocm recipes for rocm-5.5.0 and rocm-5.5.1 releases #37910

merged 24 commits into from
Jul 27, 2023

Conversation

srekolam
Copy link
Contributor

Initial commit for rocm-5.5.0 release. Planning to update this PR with rocm-5.5.1 as it is released today.

@spackbot-app
Copy link

spackbot-app bot commented May 25, 2023

@estewart08 can you review this PR?

This PR modifies the following package(s), for which you are listed as a maintainer:

  • atmi
  • comgr
  • grpc
  • hip
  • hip-rocclr
  • hipblas
  • hipcub
  • hipfft
  • hipfort
  • hipify-clang
  • hiprand
  • hipsolver
  • hipsparse
  • hsa-rocr-dev
  • hsakmt-roct
  • llvm-amdgpu
  • mesa
  • migraphx
  • miopen-hip
  • miopen-opencl
  • miopengemm
  • mivisionx
  • rccl
  • rdc
  • rocalution
  • rocblas
  • rocfft
  • rocm-bandwidth-test
  • rocm-clang-ocl
  • rocm-cmake
  • rocm-core
  • rocm-dbgapi
  • rocm-debug-agent
  • rocm-device-libs
  • rocm-gdb
  • rocm-opencl
  • rocm-openmp-extras
  • rocm-smi-lib
  • rocm-tensile
  • rocm-validation-suite
  • rocminfo
  • rocprim
  • rocprofiler-dev
  • rocrand
  • rocsolver
  • rocsparse
  • rocthrust
  • roctracer-dev
  • roctracer-dev-api
  • rocwmma

cgmb
cgmb reviewed May 25, 2023
View reviewed changes
Copy link
Contributor

@cgmb cgmb left a comment

Choose a reason for hiding this comment

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

The libraries look good. I had a couple comments on other packages, but nothing too important.

var/spack/repos/builtin/packages/hip/0014-remove-compiler-rt-linkage-for-host.5.5.0.patch Show resolved Hide resolved
var/spack/repos/builtin/packages/llvm-amdgpu/package.py Outdated Show resolved Hide resolved
@spackbot-app spackbot-app bot added the python label Jun 7, 2023
@srekolam
Copy link
Contributor Author

srekolam commented Jun 8, 2023

@spackbot rerun pipeline

cgmb
cgmb reviewed Jun 13, 2023
View reviewed changes
var/spack/repos/builtin/packages/comgr/package.py
Comment on lines +163 to +164
for ver in ["5.5.0", "5.5.1"]:
depends_on("rocm-core@" + ver, when="@" + ver)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really depend on rocm-core? That would be very unfortunate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i think so.. you can refer here . https://github.com/RadeonOpenCompute/ROCm-CompilerSupport/blob/amd-stg-open/lib/comgr/CMakeLists.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

In the deb/rpm packages, the declared dependencies on rocm-core exist only so that you can apt remove rocm-core or dnf remove rocm-core and the package manager will remove all other rocm components.

The new thing in ROCm 5.5 is that a rocmcore library was added to the rocm-core package. The library that can be used to query the version number of the rocm release. However, nothing should actually be using that library. The version APIs it provides only really make sense in the context of the AMD deb/rpm releases.

@cgmb
Copy link
Contributor

cgmb commented Jun 14, 2023 •
edited
Loading

This is a bit of a rant, but the creation of the rocmcore library is very unfortunate. There is no way to correctly use the version number that it provides outside the official AMD RPM/DEB packages. For example, on Debian Bookworm, the version of HIP is 5.2.3 but most of the math libraries are 5.3.3. If users care about the version of HIP, they should query the version of HIP and if users care about the version of, for example, rocSPARSE, they should query the version of rocSPARSE. Querying rocmcore and assuming that all components match the rocmcore version will lead to nothing but headaches when that assumption is violated.

@alecbcs alecbcs mentioned this pull request Jun 15, 2023
Closed
@srekolam
Copy link
Contributor Author

srekolam commented Jul 6, 2023

can i get some reviews and if things are okay, approval..pls.

@srekolam
Copy link
Contributor Author

@tldahlgren , can you please let me know if this can be merged?

@srekolam
Copy link
Contributor Author

@spackbot rerun pipeline

@spackbot-app
Copy link

spackbot-app bot commented Jul 17, 2023

I've started that pipeline for you!

@srekolam
Copy link
Contributor Author

@spackbot rerun pipeline

@spackbot-app
Copy link

spackbot-app bot commented Jul 24, 2023

I've started that pipeline for you!

srekolam added a commit to srekolam/spack that referenced this pull request Jul 25, 2023
@srekolam
add latest sha for mesa and limit the patches to older version.similar
cb690fd 
changes in spack#37910 to enable gitlab-ci pass
@srekolam
Copy link
Contributor Author

@tldahlgren , can you please review this and let me know if this can be merged,

tldahlgren
tldahlgren approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

Re-confirmed all of the version sha256.

@tldahlgren tldahlgren merged commit 8908b75 into spack:develop Jul 27, 2023
13 checks passed
@msimberg
Copy link
Contributor

msimberg commented Sep 8, 2023 •
edited
Loading

@srekolam sorry commenting way past this is merged, but I've been trying to get rocprof working through spack. What was the real reason for reverting the rocprofiler-dev changes inhttps://github.com//pull/37910/commits/82d45467ef5c9d29bcf3acec5cdc21859c0ef9da? rocprofiler-dev already has a patch for ignoring aqlprofile for older version: https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/rocprofiler-dev/0001-Continue-build-in-absence-of-aql-profile-lib.patch. Did the aql stuff go from being easily ignorable to being more difficult to ignore? I've seen comment about it being closed source so it makes sense that one would want to remove that dependency. I'm mainly just wondering what happened in 5.5.0 regarding that?

Edit: It seems like someone has been able to patch out the dependency also in the source code without any apparent problems: ROCm/ROCm#1781 (comment).

@srekolam
Copy link
Contributor Author

srekolam commented Sep 9, 2023

basically libhsa-amd-aqlprofile64.so has been a closed source binary and due to which we could not get that in the spack build for rocprofiler.
The patches inside the rocprofiler-dev was trying to get past the build failures that were seen due to absence of this libhsa-amd-aqlprofile64.so .
the initially patches were able to work around it but from 5.5.0 onwards there were changes to the rocprofiler-dev makefiles which made it more difficult to get past the failures.
if you have the libhsa-amd-aqlprofile64.so in the /opt/rocm-xx.xx.xx/lib path then i believe you would not need this patch.
There is also a plan to have the aqlprofile as a public git hub project but that is still under some discussion.

mpokorny pushed a commit to mpokorny/spack that referenced this pull request Sep 18, 2023
@srekolam @mpokorny
Updates to rocm recipes for rocm-5.5.0 and rocm-5.5.1 releases (spack…
ca9f418 
…#37910)

* initial commit for rocm-5.5.0 release
* fix the hipsparse  build error for 5.5.0
* fix build error for amrex .add hiprand as a dependency
* modify the patch for rocprofiler-dev
* add hiprand for +rocm build
* initial commit for rocm-5.5.1 release
* bump up the version for rocm-5.5.1 release.
* bump up the version for rocmlir.miopen to use this backend only till 5.5
* add new recipe py-barectf and add it as dependency for rocprofiler-dev
* revert the changes for rocprofiler-dev for 5.5.0/1 for now as it depends
  on hsa-amdaqlprofile.so which is a closed source and no spack recipe is
  available for now.
* add rocm-core as dependency for rocm packages from 5.5.0 onwards
* avoid download of the gtest for building unit tests
adamjstewart pushed a commit that referenced this pull request Mar 7, 2024
@srekolam
Enable tensorflow-2.11 support for ROCm (#38520)
c6fcb10 
* enable tensorflow-2.11 support for ROCm

* add latest sha for mesa and limit the patches to older version.similar
changes in #37910 to enable gitlab-ci pass

* address review commemts
mathomp4 pushed a commit to mathomp4/spack that referenced this pull request Mar 27, 2024
@srekolam @mathomp4
Enable tensorflow-2.11 support for ROCm (spack#38520)
30b24a0 
* enable tensorflow-2.11 support for ROCm

* add latest sha for mesa and limit the patches to older version.similar
changes in spack#37910 to enable gitlab-ci pass

* address review commemts
teaguesterling pushed a commit to teaguesterling/spack that referenced this pull request Jun 15, 2024
@srekolam @teaguesterling
Enable tensorflow-2.11 support for ROCm (spack#38520)
3d5ab25 
* enable tensorflow-2.11 support for ROCm

* add latest sha for mesa and limit the patches to older version.similar
changes in spack#37910 to enable gitlab-ci pass

* address review commemts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgmb cgmb cgmb left review comments

@tldahlgren tldahlgren tldahlgren approved these changes

@renjithravindrankannath renjithravindrankannath renjithravindrankannath approved these changes

@arjun-raj-kuppala arjun-raj-kuppala Awaiting requested review from arjun-raj-kuppala

@chuckatkins chuckatkins Awaiting requested review from chuckatkins

@haampie haampie Awaiting requested review from haampie

@nazavode nazavode Awaiting requested review from nazavode

@v-dobrev v-dobrev Awaiting requested review from v-dobrev

Assignees
No one assigned
Labels
dependencies new-package new-variant new-version patch python update-package virtual-dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@srekolam @cgmb @msimberg @tldahlgren @renjithravindrankannath