Page MenuHomePhabricator

[libomptarget][DeviceRTL] Generalise and simplify cmakelists
ClosedPublic

Authored by JonChesterfield on Oct 18 2021, 3:58 AM.

Details

Summary

Step towards building the DeviceRTL for amdgpu.

Mostly replaces cuda-specific toolchain finding logic with the
generic logic currently found in the amdgpu deviceRTL cmake. Also
deletes dead code and changes the default to build on systems
without cuda installed, as the library doesn't use cuda and the
amdgpu-only systems generally won't have cuda installed.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Oct 18 2021, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 3:58 AM
  • add hook to disable bitcode build as requested on the previous amdgpu lib
openmp/libomptarget/DeviceRTL/CMakeLists.txt
12

Built it by default - only depends on clang (doesn't even use freestanding libc headers at present). This was a feature request on amdgpu/deviceRTL and it's cheap to keep that alive here. Someone building libomptarget with a clang which is too old to compile this library may want to opt out.

21–22

This is copy&paste from the amdgpu cmakelists with the debug print slightly changed.

93–94

MAX_SM is cruft from the old deviceRTL, not used in the new one

97–98

these messages are redundant with the ones emitted above

178

I think it reads better to sort out the dependency for both tools before using them

186

This looks dubious - the tailing _opt corresponds to the following custom command, but we're calling opt as opt file.bc -o file.bc. Probably want to link to file.link.bc or similar then opt to file.bc so we've got both on disk after the build. I vaguely recall problems reading & writing to the same file on windows. Leaving for a later change.

openmp/libomptarget/DeviceRTL/CMakeLists.txt
142

^ this looks wrong, can anyone confirm?

I think it says that the target of all bitcode libraries depends on opt and llvm-link, however each of the libraries already depends on opt and llvm-link, using code that carefully only adds that dependency if they're part of the build.

If I'm reading this right, this line:

add_dependencies(omptarget-new-nvptx-bc opt llvm-link)

is redundant in the best case and will break the build in the worst case

openmp/libomptarget/DeviceRTL/CMakeLists.txt
212

like the above comment, I don't think we want to add opt and llvm-link as dependencies here

Aside from changing to building by default and some changes to debug messages, this is intended to be a non-functional change.

Meinersbur added inline comments.Oct 18 2021, 11:05 AM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
34–37

This removes support for LIBOMPTARGET_NVPTX_CUDA_COMPILER/LIBOMPTARGET_NVPTX_BC_LINKER settings? Instead, it uses clang found in LLVM_TOOLS_BINARY_DIR, which very much prefer over ${CMAKE_C_COMPILER}. This this work with an LLVM_DIR to a pre-installed LLVM in /usr or /usr/local?

openmp/libomptarget/DeviceRTL/CMakeLists.txt
34–37

Changes the flag yeah. This is openmp code - no particular reason why NVPTX_CUDA_COMPILER is a good name for which one to use.

The cmake will have a go with whatever LLVM_DIR is set, whether that will succeed or not depends on whether that compiler is recent enough to deal with the openmp features used. Variant is probably the limiting factor.

We're trying to encourage people to build this via ENABLE_RUNTIMES as than ensures that the clang used can actually build it, but there are still these hooks if people want to try with a different one.

Meinersbur added inline comments.Oct 18 2021, 11:40 AM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
34–37

Have you seen https://lists.llvm.org/pipermail/llvm-dev/2021-October/153238.html ?

I tried the "Default" build with openmp and it did not work:

CMake Error at /home/meinersbur/src/llvm-project/openmp/runtime/src/CMakeLists.txt:314 (string):
  string sub-command REGEX, mode MATCH needs at least 5 arguments total to
  command.
openmp/libomptarget/DeviceRTL/CMakeLists.txt
34–37

I read that earlier today but haven't tried to work out the implications for openmp.

string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION ${PACKAGE_VERSION})

I don't know offhand what that's trying to do. Seems unrelated to this patch?

Meinersbur added inline comments.Oct 18 2021, 12:04 PM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
34–37

Just asking whether we have a plan to make this work if we want to support "Default builds" as well. I think it does, since it uses the same <root>/runtimes/CMakeLists.txt that sets LLVM_DIR using:

find_package(LLVM PATHS "${LLVM_BINARY_DIR}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH)
find_package(Clang PATHS "${LLVM_BINARY_DIR}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH)

This will import LLVM/Clang's *Targets.cmake. Instead of find_program, one can get the executable path directly using IMPORTED_LOCATION.

Meinersbur added inline comments.Oct 18 2021, 12:22 PM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
34–37

The documentation also mentions that imported add_executable can be used in add_custom_command. Maybe

add_custom_command(
  clang <args> ...

would just work as well? (this would also work for in-tree clang instead of $<TARGET_FILE:clang>).

186

The OUTPUT must match the OUTPUT of the other add_custom_command on line 192 to add a DEPENDS dependency to the generation of that output dependency. Thus add_custom_command itself does not execute anything.

With the APPEND option I think this must come after the main add_custom_command line on line 192. Alternatively, you could add the DEPENDS opt to that line as well, but it would result in an error if the opt target does not exist. It actually seem to (either the in-tree target or the imported target), since otherwise add_dependencies(${bclib_target_name} opt llvm-link) would give an error es well.

I'm fairly confident that this change is correct as written because:

  • The complicated part if (LLVM_DIR)... is copied directly from amdgpu where it was debugged into existence by other people and has built by default for a long time
  • MAX_SM is unused
  • The rest is carefully renaming variable and slightly reordering clauses into a sequence that seems more likely to work than the current one

The dependency description in the current file looks inconsistent, there's a top level target that seems to be unused, and I think it's optimising a bitcode library then installing the non-optimised one. That I'm not confident to change, especially at the same time as this refactor, as the odds are high that I'm missing some aspect of how cmake or the rest of the build system behave.

I mistrust cmake's documentation as it correlates poorly with observed behaviour. I also anticipate openmp breaking in a fashion I am ill equipped to repair when the cmake changes to libc++ et al land. Simplifying the current script before semantics change seems a good move.

IMO the dependence should be, if we are using out of tree build, check the three binaries ahead of time and return error directly if anything is wrong. We don't need to set dependence from custom target to the three binaries in this mode. For in tree build, we use the target file to for the custom command, and use the target name for dependences.

Meinersbur requested changes to this revision.Oct 18 2021, 6:42 PM

Simplifying the current script before semantics change seems a good move.

This was what I am suggestion. If the current patch is working, I am ok with keeping it as-is.

However, regarding the correctness of the this patch it is required to have any

add_custom_command(<target> ... APPEND)

occur after

add_custom_command(<target> ...  COMMAND)

as by CMake's documentation:

APPEND
Append the COMMAND and DEPENDS option values to the custom command for the first output specified. There must have already been a previous call to this command with the same output.

This revision now requires changes to proceed.Oct 18 2021, 6:42 PM
  • restore original order of custom command / add depends
JonChesterfield added a comment.EditedOct 19 2021, 12:12 AM

as by CMake's documentation:

APPEND
Append the COMMAND and DEPENDS option values to the custom command for the first output specified. There must have already been a previous call to this command with the same output.

Thanks! I misread that part of your earlier comment. Restored the corresponding parts to the order they were in previously. Curious that append before define was not reported as an error by cmake.

openmp/libomptarget/DeviceRTL/CMakeLists.txt
178

^turns out dependencies come after use, not before, in this context

Ping. I'm probably going to land this series on rocm in the near future so some amd-internal people can do some testing on it. I'll have an easier time in the merges if some of it can go on trunk first

This revision is now accepted and ready to land.Oct 21 2021, 7:46 AM

Missed a comment at the previous Submit button press.

openmp/libomptarget/DeviceRTL/CMakeLists.txt
142

Redundant dependencies have no consequences, but forgetting them may result in non-deterministic failures.

I don't see how it could break the build as long as all targets exist. opt and llvm-link targets exist in both build configs, LLVM_ENABLE_PROJECTS and LLVM_ENABLE_RUNTIMES.

openmp/libomptarget/DeviceRTL/CMakeLists.txt
195

Is this stuff not here to work around opt / llvm-link sometimes not being targets?

Meinersbur added inline comments.Oct 22 2021, 11:06 AM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
195

Mostly to only add a dependency only when configured to use in-tree llvm-link/opt. llvm-link os a target in both, the LLVM_ENABLE_PROJECTS and LLVM_ENABLE_RUNTIMES (an "import target" from find_package(LLVM)) build.

openmp/libomptarget/DeviceRTL/CMakeLists.txt
195

That's why I said this here.