[Libomptarget] Don't build the device runtime without a new Clang

[jhuber6]

Summary

The OpenMP device offloading library is a bitcode library and thus only
expect to build and linked with the same version of clang that was used
to create it. This somewhat copmlicates the building process as we
require the Clang that was just built to be used to create the library.
This is either done with a two-step build, where OpenMP is built with
the Clang that was just installed, or through the
-DLLLVM_ENABLE_RUNTIMES=openmp option. This has always been the case,
but recent changes have caused this to make it difficult to build the
rest of OpenMP. This patchs adds a check to not build the OpenMP device
runtime if the current compiler is not Clang with the same version as
the LLVM installation. This should allow users to build OpenMP as a
project using any compiler without it erroring out due to the bitcode
library, but if users require it they will need to use the above methods
to compile it.

[jdoerfert]

Typos in the commit message.
s/new Clang/matching Clang/ ?

Otherwise, LGTM.

[ye-luo]

This is different from what I'm looking for. DeviceRTL should still be built as it was. Only the part added by D125315 should be disabled.

[jhuber6]

In D125698#3516179, @ye-luo wrote:

This is different from what I'm looking for. DeviceRTL should still be built as it was. Only the part added by D125315 should be disabled.

But we never supported building the bit-code library with another library in the first place, this makes that more explicit.

[ye-luo]

This is either done with a two-step build, where OpenMP is built with
the Clang that was just installed, or through the
-DLLLVM_ENABLE_RUNTIMES=openmp option. This has always been the case,

This is not true. Even before D125315 breakage, all the host libraries can be built with GCC and DeviceRTL with just-built Clang via -DLLVM_ENABLE_PROJECTS=openmp
Maybe we should figure out why D125315 doesn't pick up clang when building DeviceRTL.

[tianshilei1992]

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

[jhuber6]

In D125698#3516187, @ye-luo wrote:

This is either done with a two-step build, where OpenMP is built with
the Clang that was just installed, or through the
-DLLLVM_ENABLE_RUNTIMES=openmp option. This has always been the case,

This is not true. Even before D125315 breakage, all the host libraries can be built with GCC and DeviceRTL with just-built Clang via -DLLVM_ENABLE_PROJECTS=openmp
Maybe we should figure out why D125315 doesn't pick up clang when building DeviceRTL.

The old method called the binaries directly, the expected configurations to get this is -DLLVM_ENABLE_RUNTIMES=openmp or -DLLVM_ENABLE_PROJECTS=openmp with a newly built Clang. I'm not sure if there's a way we can try to build this with another compiler now. D125315 was a somewhat breaking change, I could just guard it manually there so you can still get the old libraries build with the clang binaries you have somewhere, but realistically it's not a supported build configuration.

[ye-luo]

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

[jhuber6]

In D125698#3516229, @ye-luo wrote:

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

I'll just guard the new code then. Even though it shouldn't break if you're using the standard builds it will probably make it less noisy for people currently using the other methods.

[tianshilei1992]

In D125698#3516229, @ye-luo wrote:

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

That's what I'm saying. We are using different mechanism to build it, which is better, more robust and correct. The reason it worked before is, it didn't use CMake's build command to build the target, like Joseph mentioned above.

[tianshilei1992]

In D125698#3516250, @jhuber6 wrote:

In D125698#3516229, @ye-luo wrote:

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

I'll just guard the new code then. Even though it shouldn't break if you're using the standard builds it will probably make it less noisy for people currently using the other methods.

Eventually we will abandon the old method, no?

[jhuber6]

In D125698#3516252, @tianshilei1992 wrote:

In D125698#3516250, @jhuber6 wrote:

In D125698#3516229, @ye-luo wrote:

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

I'll just guard the new code then. Even though it shouldn't break if you're using the standard builds it will probably make it less noisy for people currently using the other methods.

Eventually we will abandon the old method, no?

I'm not sure how long it will need to stick around, you get pretty poor performance linking in the library late without LTO on NVPTX since we can't do any inlining or constant propagation through the device runtime library. I'd like to get rid of it, but it'd definitely hit performance unless we just turned on LTO by default. I think I'll just not build the static library (Means you can't use LTO) right now if we don't have CMake configured properly, later we can figure out how to move and / or deprecate this stuff.

[ye-luo]

In D125698#3516252, @tianshilei1992 wrote:

In D125698#3516250, @jhuber6 wrote:

In D125698#3516229, @ye-luo wrote:

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

I'll just guard the new code then. Even though it shouldn't break if you're using the standard builds it will probably make it less noisy for people currently using the other methods.

Eventually we will abandon the old method, no?

The old method actually work best in the current state. You thought the runtime route is better, more robust and correct. That maybe true for release but not main. I encountered clang from main miscompiling libomptarget host code and resulted in de-referencing nullptr. So right now LLVM_ENABLE_PROJECTS works best because only the DeviceLib is built by just-built Clang. This routine also doesn't require building Clang as a separate step.

[tianshilei1992]

In D125698#3516262, @ye-luo wrote:

In D125698#3516252, @tianshilei1992 wrote:

In D125698#3516250, @jhuber6 wrote:

In D125698#3516229, @ye-luo wrote:

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

I'll just guard the new code then. Even though it shouldn't break if you're using the standard builds it will probably make it less noisy for people currently using the other methods.

Eventually we will abandon the old method, no?

The old method actually work best in the current state. You thought the runtime route is better, more robust and correct. That maybe true for release but not main. I encountered clang from main miscompiling libomptarget host code and resulted in de-referencing nullptr. So right now LLVM_ENABLE_PROJECTS works best because only the DeviceLib is built by just-built Clang. This routine also doesn't require building Clang as a separate step.

I didn't say runtime routine is like that. What I were saying is, using CMake's build command to build a target is better. LLVM_ENABLE_PROJECTS in fact has nothing to do with this patch.

[jhuber6]

In D125698#3516262, @ye-luo wrote:

The old method actually work best in the current state. You thought the runtime route is better, more robust and correct. That maybe true for release but not main. I encountered clang from main miscompiling libomptarget host code and resulted in de-referencing nullptr. So right now LLVM_ENABLE_PROJECTS works best because only the DeviceLib is built by just-built Clang. This routine also doesn't require building Clang as a separate step.

Is this a recurrent problem? Or did someone just break something upstream. If you encounter problems building libomptarget with clang we should definitely try to fix it.

[jhuber6]

Changing the guard to only be for the static library.

[protze.joachim]

Are the llvm version variables defined for stand-alone builds? So, will stand-alone builds of OpenMP continue to work and include libomptarget code?

Does the compiler check the version of the bc files for compatibility during compilation of an offloading application and fail with a compile time error?

[jhuber6]

In D125698#3516297, @protze.joachim wrote:

Are the llvm version variables defined for stand-alone builds? So, will stand-alone builds of OpenMP continue to work and include libomptarget code?

Does the compiler check the version of the bc files for compatibility during compilation of an offloading application and fail with a compile time error?

That's a good question, I'm not sure how this will be handled with standalone builds considering the version check. We could probably just ignore that part if those variables aren't defined.

Typically if the writer metadata doesn't match the reader it will throw an error. I'm not sure what the granularity is, I've only ever noticed it happen between major versions. But I'm not sure how common it is to happen here.

[tianshilei1992]

openmp/libomptarget/CMakeLists.txt

88

not exactly same as before

[tianshilei1992]

In D125698#3516297, @protze.joachim wrote:

Are the llvm version variables defined for stand-alone builds? So, will stand-alone builds of OpenMP continue to work and include libomptarget code?

I'm always using standalone build. No problem. However, that might depend on how you build it. I always set the CMake compilers to what I just build. Actually I never use runtime or project build because they are less flexible.

[jhuber6]

Remove unrelated change.

[jhuber6]

@ye-luo Can I land this now?

[protze.joachim]

openmp/libomptarget/DeviceRTL/CMakeLists.txt

269 ↗

(On Diff #429763)

This line will be skipped, if the return hits. Is this the intended behavior?

[jhuber6]

openmp/libomptarget/DeviceRTL/CMakeLists.txt

269 ↗

(On Diff #429763)

Yes, that directly is only used by the above target library. The previous method just grabbed all the sources directly.

[ye-luo]

Restored gcc build in my tests.

openmp/libomptarget/DeviceRTL/CMakeLists.txt

269 ↗

(On Diff #429763)

I think the way of building omptarget.devicertl needs to be changed. It is doing differently from others and confusing.

[ye-luo]

openmp/libomptarget/DeviceRTL/CMakeLists.txt

253 ↗

(On Diff #429763)

54e02179b33f6bfd1c7bd836e790effd5ffb715c changed this line and -Xopenmp-target=nvptx64-nvidia-cuda --cuda-feature=+ptx61 doesn't seem making sense for amdgpu.

262 ↗

(On Diff #429763)

This target doesn't get added like other custom target is likely the source of all the problems.

openmp/libomptarget/CMakeLists.txt

 set(LIBOMPTARGET_OPENMP_HEADER_FOLDER "${LIBOMP_INCLUDE_DIR}" CACHE STRING
   "Path to folder containing omp.h")
 set(LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER "${LIBOMP_LIBRARY_DIR}" CACHE STRING
   "Path to folder containing libomp.so, and libLLVMSupport.so with profiling enabled")
 
 
 # Build offloading plugins and device RTLs if they are available.
 add_subdirectory(plugins)
-add_subdirectory(DeviceRTL)
 add_subdirectory(tools)
 
+set(LIBOMPTARGET_LLVM_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
+if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL LIBOMPTARGET_LLVM_VERSION)
+  add_subdirectory(DeviceRTL)
+else()
+  libomptarget_say("Not building DeviceRTL, CMake compiler '${CMAKE_CXX_COMPILER} ${CMAKE_CXX_COMPILER_VERSION}' is not 'Clang ${LIBOMPTARGET_LLVM_VERSION}'.")
+  libomptarget_say("  Use the 'LLVM_ENABLE_RUNTIMES=openmp' option instead")
+endif()
+
 # Add tests.
 add_subdirectory(test)