This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/222

@danieldk You are selling yourself short: this cleanup you've done is fantastic! So many things that I stumbled around trying to figure out and fix, you've made comprehensible. I would very much like to upstream these changes into nixos-rocm if we are indeed going to continue using the overlay for development.

High-level comments:

• Should we rename rocr and roct? These are the prefixes of their upstream repositories, but we could name them rocm-runtime and rocm-thunk or something. Confusion with rocclr seems hard to avoid, and I like the short names, but if we did want to change them now is a good time.

• Do we need llvm-outputs.patch? I understand it is from the nixpkgs LLVM packaging, but I'm not sure I understand how it comes into play.

• I think we should include hip-clang here, if possible. It is a small build, and arguably the key piece of the ROCm stack since it makes building CUDA source so straightforward. It used to be quite the circus to build it, but as of 3.5 it is a much better story.

• How do you want to handle rocr-ext? This is very often a key issue for OpenCL development since it is required for Image support. The huge issue is that it is not open source. If we don't have it here, I worry that would-be nixpkgs users of OpenCL would be forced to turn to nixos-rocm before they are able to do anything. Additionally, support for darktable, for example, requires that image support. A large swath of the broader OpenCL ecosystem needs image support, and we do have it in nixos-rocm, but with the caveat that there is the proprietary software question.

• We probably need some documentation associated with configuring NixOS to use ROCm. Things like having to reboot to get OpenCL working (I think this is still the case), are perennial gotchas. Perhaps this belongs in Chapter 16 of the nixpkgs manual.

Again, I am so impressed by and grateful for the work you've done here in cleaning up these derivations. What was before an instance of shipping the MVP is here something much closer to a designed and thought-through artifact.

Thanks for all the feedback, I have pushed updated commits.

I would very much like to upstream these changes into nixos-rocm if we are indeed going to continue using the overlay for development.

Once this is merged in nixpkgs I can do a PR for nixos-rocm to incorporate the changes.

* Should we rename `rocr` and `roct`? These are the prefixes of their upstream repositories, but we could name them `rocm-runtime` and `rocm-thunk` or something. Confusion with `rocclr` seems hard to avoid, and I like the short names, but if we did want to change them now is a good time.

That's indeed a lot less confusing, so I renamed these two derivations.

* Do we need `llvm-outputs.patch`? I understand it is from the `nixpkgs` LLVM packaging, but I'm not sure I understand how it comes into play.

I think we still need this, because dynamic libraries are in the separate lib output. It is probably useful for reducing the closure size, since the final OpenCL runtime only uses the lib output:

❯ nix-store -qR result | grep llvm
/nix/store/cy5c6z1pxwr2r40wslqhc4k732vp7nwd-rocm-llvm-3.5.1-lib

* I think we should include `hip-clang` here, if possible. It is a small build, and arguably the key piece of the ROCm stack since it makes building CUDA source so straightforward. It used to be quite the circus to build it, but as of 3.5 it is a much better story.

Sounds good. I think this PR is already quite big. So once it's merged, I can look at hip-clang to.

* How do you want to handle `rocr-ext`? This is very often a key issue for OpenCL development since it is required for Image support. The huge issue is that it is not open source. If we don't have it here, I worry that would-be nixpkgs users of OpenCL would be forced to turn to `nixos-rocm` before they are able to do anything. Additionally, support for `darktable`, for example, requires that image support. A large swath of the broader OpenCL ecosystem needs image support, and we do have it in `nixos-rocm`, but with the caveat that there is the proprietary software question.

We should probably package that. In general, closed-source software is ok for inclusion, but we cannot let ofborg/Hydra build it. We do have to check the EULA to see what the exact requirements are (e.g. some packages like Oracle's JDK require the user to download the package themselves and add it to the store because they need to confirm the EULA explicitly). I can look into that after this PR.

* We probably need some documentation associated with configuring NixOS to use ROCm. Things like having to reboot to get OpenCL working (I think this is still the case), are perennial gotchas. Perhaps this belongs in Chapter 16 of the nixpkgs manual.

Agreed. Also about configuring NixOS with OpenCL (and third-party Vulkan) in general. Another possibility would be Part II of the NixOS manual:

https://nixos.org/nixos/manual/index.html#ch-configuration

Thanks again for all the comments and feedback!

@matthewbauer since your are an LLVM codeowner, any objections against merging this?

@Flakebi I think you are also an amdgpu user, since you recently contributed the amdvlk derivation? Any chance you could also give this PR a quick look?

Thanks for the note @danieldk, I compiled two small OpenCL examples from the ROCm docs and it worked great (once I set OCL_ICD_VENDORS=results/rocm-opencl-icd/etc/OpenCL/vendors/, the trailing slash is important).

Thank you for your work!

Thanks for the note @danieldk, I compiled two small OpenCL examples from the ROCm docs and it worked great (once I set OCL_ICD_VENDORS=results/rocm-opencl-icd/etc/OpenCL/vendors/, the trailing slash is important).

Thanks for testing!

I guess I’m a little bit more than a user ;)

Whoops, didn't notice that you are involved in GPUOpen. 😲 😃

@Flakebi You should be able to forego that environment variable if you install the package, specifically the hardware.opengl.extraPackages = [ pkgs.rocm-opencl-icd ]; part.

Two thumbs up, no complaints, this should be ready to go then.