rocm-openmp-extras: do not mutate llvm-amdgpu

[haampie]

rocm-openmp-extras shouldn't make changes to the prefix of llvm-amdgpu.

Ref #48941

[spackbot-app]

@estewart08 can you review this PR?

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

• rocm-openmp-extras

[estewart08]

At first glance it looks like running an openmp or fortran program will fail due to these links being gone. Is it feasible that these links are created in llvm-amdgpu? If not, is there another recommendation? In a non-spack build, flang executables will be installed into the bin directory of llvm. Due to spack having separate install prefixes, this workaround was needed. Similar story for the libdevice directory (which will no longer exist when a future ROCm brings in that update).

[haampie]

Normally openmp is built as a runtime during the llvm build (just built clang builds runtimes) so these things stay in the same prefix. I don't really get the rocm-openmp-extras package to be honest.

Notice that changes made by rocm-openmp-extras to llvm-amdgpu are not persisted for binaries we distribute, meaning that probably the package is broken.

Would you be able to improve this? I just submitted the PR to highlight the problem.

[afzpatel]

Is it necessary to use flang-legacy for rocm-openmp-extras? If we could use flang-new built from llvm we won't need the flang symlinks.

[estewart08]

Is it necessary to use flang-legacy for rocm-openmp-extras? If we could use flang-new built from llvm we won't need the flang symlinks.

Currently, yes it is. Flang-new is in a preproduction state and is not in ROCm yet.

[estewart08]

Normally openmp is built as a runtime during the llvm build (just built clang builds runtimes) so these things stay in the same prefix. I don't really get the rocm-openmp-extras package to be honest.

Notice that changes made by rocm-openmp-extras to llvm-amdgpu are not persisted for binaries we distribute, meaning that probably the package is broken.

Would you be able to improve this? I just submitted the PR to highlight the problem.

Upstream you are correct, the default build method is to use LLVM_ENABLE_RUNTIMES=openmp;offload. ROCm does not yet use that. It instead uses the "standalone" build of openmp/offload, which invokes cmake directly in the openmp/offload directories. So that is what rocm-openmp-extras does along with flang-classic support.

That being said, I do want to move ROCm to use LLVM_ENABLE_RUNTIMES.

[haampie]

Sounds like you want to build flang as part of llvm whether it's "legacy" or flang-new/flang.

As for openmp, I don't see why it's removed from llvm-amdgpu, you can run cmake(...) there? We still do that for old llvm versions for cuda:

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

Lines 1194 to 1225 in fcc85ad

	@run_after("install")
	def post_install(self):
	spec = self.spec
	define = self.define
	# unnecessary if we build openmp via LLVM_ENABLE_RUNTIMES
	if self.spec.satisfies("+cuda openmp=project"):
	ompdir = "build-bootstrapped-omp"
	prefix_paths = get_cmake_prefix_path(self)
	prefix_paths.append(str(spec.prefix))
	# rebuild libomptarget to get bytecode runtime library files
	with working_dir(ompdir, create=True):
	cmake_args = [
	"-G",
	"Ninja",
	define("CMAKE_BUILD_TYPE", spec.variants["build_type"].value),
	define("CMAKE_C_COMPILER", spec.prefix.bin + "/clang"),
	define("CMAKE_CXX_COMPILER", spec.prefix.bin + "/clang++"),
	define("CMAKE_INSTALL_PREFIX", spec.prefix),
	define("CMAKE_PREFIX_PATH", prefix_paths),
	]
	cmake_args.extend(self.cmake_args())
	cmake_args.extend(
	[
	define("LIBOMPTARGET_NVPTX_ENABLE_BCLIB", True),
	self.stage.source_path + "/openmp",
	]
	)
	cmake(*cmake_args)
	ninja()
	ninja("install")

[estewart08]

Sounds like you want to build flang as part of llvm whether it's "legacy" or flang-new/flang.

Flang-new definitely will be built as a part of llvm.

The goal, at least when I created the rocm-openmp-extras recipe, was to mimic the ROCm build. At the time the LLVM_ENABLE_RUNTIMES was not adopted upstream. The build of openmp/offload has dependencies on rocr and rocm-device-libs. You can get around this by building both rocr/rocm-device-libs via LLVM_EXTERNAL_PROJECTS, which I think spack already builds rocm-device-libs that way.

I would prefer to do the bare minimum to fix this issue: #48941. This is why I suggested creating those links in the llvm-amdgpu build.

Then when ROCm switches to LLVM_ENABLE_RUNTIMES for openmp/offload we can make the larger adjustments to the recipes along with the flang-new changes.

[haampie]

The only time you can put files in the llvm-amdgpu prefix is when you build llvm-amdgpu. So, if it's strictly necessary to have <llvm amdgpu prefix>/bin/flang to build other things, then you must build flang as part of llvm-amdgpu.

You cannot and should not expect the dependency's prefix to be writable (e.g. upstreams, externals, future Spack features).

[github-actions]

This pull request has been automatically marked as stale because it has not had any activity in the last 6 months. It will be closed in 30 days if there is no further activity.
If the pull request is waiting for a reply from reviewers, feel free to ping them as a reminder. If it is waiting and has no assigned reviewer, feel free to ping @spack/spack-releasers or simply leave a comment saying this should not be marked stale. This will reset the pull request's stale state.
To get more eyes on your pull request, you can post a link in the #pull-requests channel of the Spack Slack.
Thank you for your contributions!

[github-actions]

This pull request was closed because it had no activity for 30 days after being marked stale. If you feel this is in error, please feel free to reopen this pull request.