This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jhuber6 on May 16 2022, 8:55 AM.

Details

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.

Diff Detail

Event Timeline

jhuber6 created this revision.May 16 2022, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 8:55 AM
Herald added a subscriber: mgorny. · View Herald Transcript
jhuber6 requested review of this revision.May 16 2022, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 8:55 AM
jdoerfert accepted this revision.May 16 2022, 9:00 AM

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

Otherwise, LGTM.

This revision is now accepted and ready to land.May 16 2022, 9:00 AM

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.

ye-luo requested changes to this revision.May 16 2022, 9:01 AM
This revision now requires changes to proceed.May 16 2022, 9:01 AM

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.

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 accepted this revision.EditedMay 16 2022, 9:09 AM

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.

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.

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

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.

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.

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?

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.

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.

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.

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 updated this revision to Diff 429750.May 16 2022, 9:46 AM

Changing the guard to only be for the static library.

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?

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 added inline comments.May 16 2022, 9:56 AM
openmp/libomptarget/CMakeLists.txt
88 ↗(On Diff #429750)

not exactly same as before

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 updated this revision to Diff 429763.May 16 2022, 10:21 AM

Remove unrelated change.

@ye-luo Can I land this now?

openmp/libomptarget/DeviceRTL/CMakeLists.txt
269

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

jhuber6 added inline comments.May 16 2022, 12:09 PM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
269

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

ye-luo accepted this revision.May 16 2022, 12:46 PM

Restored gcc build in my tests.

openmp/libomptarget/DeviceRTL/CMakeLists.txt
269

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

This revision is now accepted and ready to land.May 16 2022, 12:46 PM
ye-luo added inline comments.May 16 2022, 12:52 PM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
253

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

262

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