Rocm 5.2 release integration by renjithravindrankannath · Pull Request #31591 · spack/spack

renjithravindrankannath · 2022-07-15T02:56:27Z

This PR contains changes for ROCM 5.2.0 release.
Additional changes for hipify-clang and rocm-openmp-extras will be under different PR

renjithravindrankannath · 2022-07-15T20:25:54Z

Installed all component successfully.
[email protected]

Unit test result in hip below.
hip_test_logs.txt

rocm-bandwidth-test result below.
rocm-bandwidth-test_test_logs.txt

var/spack/repos/builtin/packages/rocsolver/package.py

var/spack/repos/builtin/packages/hipsolver/package.py

var/spack/repos/builtin/packages/aomp/package.py

var/spack/repos/builtin/packages/hipfft/package.py

var/spack/repos/builtin/packages/rocblas/package.py

var/spack/repos/builtin/packages/rocm-debug-agent/package.py

var/spack/repos/builtin/packages/rocrand/package.py

srekolam

request the changes

var/spack/repos/builtin/packages/rocblas/package.py

cgmb

There's just one more thing in the rocblas package that I see.

var/spack/repos/builtin/packages/rocblas/package.py

srekolam

LGTM!!

srekolam · 2022-07-25T20:55:32Z

@haampie , @tldahlgren can you approve this PR and let us know if this can be merged

srekolam · 2022-07-26T22:15:08Z

@haampie or @tldahlgren can you approve this PR. i am planning to get 5.2.1 after this PR merge.

cgmb

It would be nice to get #31450 merged so that changes like these are automatically tested.

srekolam · 2022-08-06T00:40:33Z

@alalazo or @tldahlgren can we get this merged now

alalazo

I did a first pass at reading the PR. I didn't comment on each place since I think there would be a lot of repetitions in my comments. Basically:

There are a lot of patches that I'd really like to see moving upstream
The patch directives usually don't have comments to explain what they do, who produced the patch etc.
The patches themselves don't have provenance

This wouldn't make life easy for whomever reads these package.py next. Can we handle these issues?

Note that I didn't comment on functional changes, since I don't know much about the rocm stack, so I'll defer that to other maintainers/core-developers.

var/spack/repos/builtin/packages/atmi/package.py

alalazo · 2022-08-08T07:37:15Z

var/spack/repos/builtin/packages/ginkgo/package.py

                args.append("-DGINKGO_HIP_AMDGPU={0}".format(arch_str))
+            if spec.satisfies("^[email protected]:"):
+                args.append(
+                    "-DCMAKE_MODULE_PATH={0}/".format(self.spec["hip"].prefix.lib.cmake.hip)


Is the additional / needed?

Removed the / in the recipe.

alalazo · 2022-08-08T07:39:27Z

var/spack/repos/builtin/packages/hip/package.py

+        when="@5.0.2:5.1.3",
+    )
+
+    patch(
+        "0012-Improve-compilation-without-git-repo-and-remove-compiler-rt-linkage-for-host"
+        ".5.2.0.patch",
+        when="@5.2.0:",


Same comments as above:

It would be better is this is fixed upstream

In any case a comment explaining what the patch does, why is it needed and where it was taken from is welcome to help the next reader of package.py

@alalazo I have been working with the team to get this changes into the next release . While 5.2.1 will not have this change, i would see that this is addressed in 5.3.0 release.

The compilation without git was fixed in ROCm/hipamd@56b3260 and is on track for ROCm 5.3. If you have a fix for the rt linkage, that's great!

var/spack/repos/builtin/packages/hipblas/package.py

var/spack/repos/builtin/packages/hipcub/package.py

var/spack/repos/builtin/packages/hipfft/package.py

var/spack/repos/builtin/packages/migraphx/package.py

var/spack/repos/builtin/packages/rocalution/package.py

var/spack/repos/builtin/packages/rocblas/package.py

the new patches applied in rocm components

renjithravindrankannath · 2022-08-09T23:42:28Z

Hi @alalazo ,
I have added the changes as per you review comment. Could you please review and approve if everything is good.

alalazo

Changing the maintainers and adding tags can be extracted to a separate PR. I think that wouldn't require a lot of reviews to be merged. There are other comments below, always related to patches.

If I understand correctly this package is maintained by AMD people. May I ask why AMD is releasing versions of the Rocm stack that are then heavily patched afterwards in Spack?

var/spack/repos/builtin/packages/atmi/package.py

var/spack/repos/builtin/packages/hip/package.py

var/spack/repos/builtin/packages/migraphx/package.py

var/spack/repos/builtin/packages/rocblas/package.py

var/spack/repos/builtin/packages/rocfft/package.py

var/spack/repos/builtin/packages/rocm-opencl/package.py

alalazo · 2022-08-10T07:45:17Z

var/spack/repos/builtin/packages/rocm-smi/package.py

    url = "https://github.com/RadeonOpenCompute/ROC-smi/archive/rocm-4.1.0.tar.gz"

-    maintainers = ["srekolam", "arjun-raj-kuppala"]
+    maintainers = ["srekolam", "renjithravindrankannath"]


Change in maintainers and tags can be a separate PR.

haampie

Just a few comments:
1.Please use define(var, val) and for ON/OFF you can use Python's booleans so that you get -Dvar:BOOL=val.
2. If you wanna change the behavior at 5.2.0, then use @:5.1 for < 5.2.0 and @5.2.0: for >= 5.2.0 so there's no possible versions between 5.1.3 - 5.2.0 (which guards against someone adding a 5.1.3-my-custom-version suffix or so).

var/spack/repos/builtin/packages/hipcub/package.py

var/spack/repos/builtin/packages/hipfft/package.py

var/spack/repos/builtin/packages/hipsparse/package.py

var/spack/repos/builtin/packages/mivisionx/package.py

var/spack/repos/builtin/packages/rocm-debug-agent/package.py

var/spack/repos/builtin/packages/rocm-opencl/package.py

var/spack/repos/builtin/packages/rocm-smi-lib/package.py

var/spack/repos/builtin/packages/hipblas/package.py

renjithravindrankannath · 2022-08-11T23:52:51Z

@alalazo , @haampie
Updated based on the review comments and all checks have passed.
Please review and approve if no more changes required.

cgmb reviewed Jul 18, 2022

View reviewed changes

var/spack/repos/builtin/packages/rocsolver/package.py Outdated Show resolved Hide resolved

var/spack/repos/builtin/packages/rocsolver/package.py Outdated Show resolved Hide resolved

var/spack/repos/builtin/packages/hipsolver/package.py Outdated Show resolved Hide resolved