refactor discovery, ship therock tarballs as artifacts - #1024
Draft
simeonschaub wants to merge 4 commits into
Draft
refactor discovery, ship therock tarballs as artifacts#1024simeonschaub wants to merge 4 commits into
simeonschaub wants to merge 4 commits into
Conversation
…ry subpackages By default the ROCm runtime (HIP, HSA) and vendor libraries (rocBLAS, rocSPARSE, rocSOLVER, rocRAND, rocFFT, MIOpen) now come from the new ROCm_Runtime subpackage, which ships AMD's TheRock distribution tarballs directly as lazy artifacts, selected by GPU architecture and ROCm version through platform augmentation. The former src/discovery/ code moves into the ROCm_Runtime_Discovery subpackage, which replicates ROCm_Runtime's API for a local ROCm installation; it is only loaded when that is requested through ROCm_Runtime's "local" preference, settable via the new AMDGPU.set_rocm_version!([version]; local_rocm) (mirroring CUDA.set_runtime_version!). ld.lld now always comes from AMDGPU_LLVM_Backend_jll (it has to match the LLVM version device code is generated with), and the device bitcode libraries keep coming from ROCmDeviceLibs_jll until local ones can be downgraded. libMIOpen_path is renamed to libMIOpen for consistency with the provider packages. Also adds a manually-dispatched workflow to regenerate the Artifacts.toml hashes from repo.amd.com. Assisted-by: Claude Code (claude-fable-5)
Both ROCm_Runtime and ROCm_Runtime_Discovery export the same library path globals, so AMDGPU can simply `using` whichever the "local" preference selects: the imported bindings alias the provider's globals and pick up the values assigned by its `__init__`, which runs before AMDGPU loads. AMDGPU's `libhip`/`libhsaruntime` names are renamed to the providers' `libamdhip64`/`libhsa_runtime64` so no aliasing layer is needed, and `lld_path`/`libdevice_libs` are imported directly from their JLLs, which are assumed to always be available. The discovery package gains libhiprtc and libamd_comgr lookups for API parity with ROCm_Runtime. As a side effect the device libraries are now available while AMDGPU itself precompiles (the JLL's `__init__` runs then), so the warming compile links them for real; the DEVICE_LIBS reset stays to keep absolute artifact paths and bitcode blobs out of the package image. Assisted-by: Claude Code (claude-fable-5)
The subpackages are wired up through [sources], which only the active project's Project.toml provides and which Pkg supports only on Julia 1.11+. Steps running on Julia 1.10 (like the GPU-less environment check) or resolving from another project (docs, perf) therefore failed with "expected package ROCm_Runtime to be registered". Explicitly Pkg.develop'ing the subpackages records path entries in the manifest, which works everywhere. Verified by simulating the CI resolve in a fresh clone on Julia 1.10 and nightly, including a GPU-less load (arch = "none") on 1.10. Assisted-by: Claude Code (claude-fable-5)
Contributor
There was a problem hiding this comment.
AMDGPU.jl Benchmarks
Details
| Benchmark suite | Current: b528fb0 | Previous: 39fe271 | Ratio |
|---|---|---|---|
amdgpu/synchronization/context/device |
510 ns |
560 ns |
0.91 |
amdgpu/synchronization/stream/blocking |
227.5 ns |
230 ns |
0.99 |
amdgpu/synchronization/stream/nonblocking |
292.5 ns |
315 ns |
0.93 |
array/accumulate/Float32/1d |
79728.75 ns |
70043.5 ns |
1.14 |
array/accumulate/Float32/dims=1 |
280644 ns |
284709 ns |
0.99 |
array/accumulate/Float32/dims=1L |
94213.75 ns |
80278.75 ns |
1.17 |
array/accumulate/Float32/dims=2 |
92308.75 ns |
72131 ns |
1.28 |
array/accumulate/Float32/dims=2L |
2752908 ns |
2614542.5 ns |
1.05 |
array/accumulate/Int64/1d |
83998.75 ns |
78386.25 ns |
1.07 |
array/accumulate/Int64/dims=1 |
248076.25 ns |
244018.5 ns |
1.02 |
array/accumulate/Int64/dims=1L |
100861.5 ns |
83741.25 ns |
1.20 |
array/accumulate/Int64/dims=2 |
91543.75 ns |
84758.75 ns |
1.08 |
array/accumulate/Int64/dims=2L |
2896302.5 ns |
2891796.75 ns |
1.00 |
array/broadcast |
70253.5 ns |
72998.75 ns |
0.96 |
array/construct |
2270 ns |
2182.5 ns |
1.04 |
array/copy |
36083 ns |
36628 ns |
0.99 |
array/copyto!/cpu_to_gpu |
111256.5 ns |
110909.25 ns |
1.00 |
array/copyto!/gpu_to_cpu |
111524.25 ns |
110519.25 ns |
1.01 |
array/copyto!/gpu_to_gpu |
39245.5 ns |
45368.25 ns |
0.87 |
array/iteration/findall/bool |
142712 ns |
132337 ns |
1.08 |
array/iteration/findall/int |
149427.25 ns |
151897.25 ns |
0.98 |
array/iteration/findfirst/bool |
150662 ns |
144392.25 ns |
1.04 |
array/iteration/findfirst/int |
146364.75 ns |
145337.25 ns |
1.01 |
array/iteration/findmin/1d |
130352 ns |
113169 ns |
1.15 |
array/iteration/findmin/2d |
116556.75 ns |
108861.75 ns |
1.07 |
array/iteration/logical |
238998.5 ns |
251833.5 ns |
0.95 |
array/iteration/scalar |
297757 ns |
295024.25 ns |
1.01 |
array/permutedims/2d |
70998.5 ns |
71828.5 ns |
0.99 |
array/permutedims/3d |
70558.75 ns |
69728.5 ns |
1.01 |
array/permutedims/4d |
73516 ns |
74073.75 ns |
0.99 |
array/random/rand/Float32 |
44463.25 ns |
45320.75 ns |
0.98 |
array/random/rand/Int64 |
53518.5 ns |
54208.25 ns |
0.99 |
array/random/rand!/Float32 |
65286 ns |
64303.25 ns |
1.02 |
array/random/rand!/Int64 |
72906 ns |
71623.5 ns |
1.02 |
array/random/randn/Float32 |
80411.25 ns |
79463.75 ns |
1.01 |
array/random/randn!/Float32 |
76156 ns |
80086.25 ns |
0.95 |
array/reductions/mapreduce/Float32/1d |
102851.5 ns |
97711.5 ns |
1.05 |
array/reductions/mapreduce/Float32/dims=1 |
93976.5 ns |
85676.25 ns |
1.10 |
array/reductions/mapreduce/Float32/dims=1L |
839882.5 ns |
832249.5 ns |
1.01 |
array/reductions/mapreduce/Float32/dims=2 |
98701.5 ns |
84791.25 ns |
1.16 |
array/reductions/mapreduce/Float32/dims=2L |
143789.75 ns |
144512 ns |
1.00 |
array/reductions/mapreduce/Int64/1d |
90721.25 ns |
97688.75 ns |
0.93 |
array/reductions/mapreduce/Int64/dims=1 |
92966.5 ns |
84121.25 ns |
1.11 |
array/reductions/mapreduce/Int64/dims=1L |
839907.5 ns |
836152 ns |
1.00 |
array/reductions/mapreduce/Int64/dims=2 |
97536.5 ns |
84931.25 ns |
1.15 |
array/reductions/mapreduce/Int64/dims=2L |
144387 ns |
142327 ns |
1.01 |
array/reductions/reduce/Float32/1d |
105276.5 ns |
97861.25 ns |
1.08 |
array/reductions/reduce/Float32/dims=1 |
93909 ns |
84426.25 ns |
1.11 |
array/reductions/reduce/Float32/dims=1L |
839620 ns |
835814.5 ns |
1.00 |
array/reductions/reduce/Float32/dims=2 |
98711.25 ns |
84691.25 ns |
1.17 |
array/reductions/reduce/Float32/dims=2L |
143722.25 ns |
143922.25 ns |
1.00 |
array/reductions/reduce/Int64/1d |
91489 ns |
97814 ns |
0.94 |
array/reductions/reduce/Int64/dims=1 |
94291.25 ns |
83881.25 ns |
1.12 |
array/reductions/reduce/Int64/dims=1L |
840957.25 ns |
827999.25 ns |
1.02 |
array/reductions/reduce/Int64/dims=2 |
97341.5 ns |
84931 ns |
1.15 |
array/reductions/reduce/Int64/dims=2L |
144492 ns |
144367 ns |
1.00 |
array/reverse/1d |
44928 ns |
45648.25 ns |
0.98 |
array/reverse/1dL |
75548.5 ns |
73036 ns |
1.03 |
array/reverse/1dL_inplace |
80503.75 ns |
80503.75 ns |
1 |
array/reverse/1d_inplace |
53133.25 ns |
52233.25 ns |
1.02 |
array/reverse/2d |
49915.75 ns |
50390.75 ns |
0.99 |
array/reverse/2dL |
90178.75 ns |
82971.25 ns |
1.09 |
array/reverse/2dL_inplace |
92281.5 ns |
92171.25 ns |
1.00 |
array/reverse/2d_inplace |
51193.25 ns |
55451 ns |
0.92 |
array/sorting/1d |
335237.5 ns |
335057.25 ns |
1.00 |
integration/byval/reference |
39201 ns |
39661 ns |
0.99 |
integration/byval/slices=1 |
40350 ns |
40411 ns |
1.00 |
integration/byval/slices=2 |
151592 ns |
147452 ns |
1.03 |
integration/byval/slices=3 |
239773 ns |
248133 ns |
0.97 |
integration/volumerhs |
5007693 ns |
5013333 ns |
1.00 |
kernel/indexing |
57733.5 ns |
57200.75 ns |
1.01 |
kernel/indexing_checked |
29160.25 ns |
54958.25 ns |
0.53 |
kernel/launch |
1347.5 ns |
1382.5 ns |
0.97 |
kernel/rand |
107264 ns |
98641.5 ns |
1.09 |
latency/import |
1990511138 ns |
1895221133 ns |
1.05 |
latency/precompile |
41933425309 ns |
38320507704 ns |
1.09 |
latency/ttfp |
6043035186 ns |
5930819959 ns |
1.02 |
This comment was automatically generated by workflow using github-action-benchmark.
The previous fix dev'ed the subpackages in the step command, but the julia-test plugin instantiates the project in its pre-command hook, which runs before the step's command -- so Julia 1.10 (no [sources] support) still failed with "expected package ROCm_Runtime to be registered". Follow CUDA.jl's approach instead: on 1.10 (tests, Enzyme, GPU-less check) drop the julia-test plugin and run Pkg.develop + Pkg.test manually; on 1.11+ the plugin works as-is since Pkg honors [sources] there, so those steps go back to their plain Pkg.update() command. Verified on a fresh clone with Julia 1.10: develop + update resolves, and Pkg.test constructs its sandbox with the dev'ed subpackages from the parent manifest. Assisted-by: Claude Code (claude-fable-5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some of the AI comments still need to be cleaned up but the logic looks reasonable from a first review, so I thought I'd put this up as a draft for feedback.
It turns out MIOpen needs all of libllvm and libclang, so unlike discussed in #919 (comment), I went back to shipping therock artifacts directly as done in that PR, since we don't gain much by repackaging anymore.
Otherwise, do a split similar to CUDA.jl of having ROCm_Runtime be a pseudo-jll providing automatic downloads through artifacts and having manual discovery live in ROCm_Runtime_Discovery. Either one of those is then loaded depending on a preference