Page MenuHomePhabricator

[OpenMP] Disable libomptarget profiling by default if built via the "runtimes" setup
ClosedPublic

Authored by mstorsjo on Nov 17 2021, 5:18 AM.

Details

Summary

In the "runtimes" setup, the runtime (e.g. OpenMP) can be built for
a target entirely different from the current host build (where LLVM
and Clang are built). If profiling is enabled, libomptarget links
against LLVMSupport (which only has been built for the host).

Thus, don't enable profiling by default in this setup.

This should allow relanding D113253.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 17 2021, 5:18 AM
mstorsjo requested review of this revision.Nov 17 2021, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 5:18 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
protze.joachim added inline comments.
openmp/CMakeLists.txt
60

For the cross-compilation use case, shouldn't the question be whether the code is compiled for Windows/MacOS rather than compiled on Windows/MacOS?

65

Can we check, whether the target architectures for llvm and runtimes are the same? In that case, building with profiling sounds reasonable.

mstorsjo added inline comments.Nov 17 2021, 12:27 PM
openmp/CMakeLists.txt
60

In CMake, APPLE and WIN32 refer to the target of the current cross compiler, not the host running the compilation. So you can do e.g. if (WIN32) # add windows specific sources endif() which is safe and correct wrt cross compilation.

65

I guess we could, but I'm afraid it could become brittle.

The situation that prompted this was that the main LLVM build and the runtimes disagreed on whether zlib was found, even if they were targeting the exact same OS/architecture. (They disagreed because the runtimes build used/will use a slightly different cmake setup, which makes the library probing behave slightly differently - also in general, runtime libraries shouldn't need to probe for external dependencies). As the main LLVM setup had zlib (which was a dependency of LLVMSupport) but the runtimes build didn't, libopenmptarget failed to include the LLVMSupport dependency.

I'm just suggesting changing the default here - one can still request it enabled (and see if it actually works or not).

The main issue as I see it, is that when building via the runtimes setup, using LLVMSupport from a runtime is a bit of a layering violation/inversion - the main overall direction is to untangle the runtimes from the main LLVM build as much as possible, that's at least what @phosek has suggested.

If building via LLVM_ENABLE_PROJECTS, so that the openmp runtime is built with the same compiler and configuration, alongside LLVM itself, using LLVMSupport is fine. Although, at least libcxx/libcxxabi/libunwind are entirely deprecating building them that way, but I'm not sure if openmp has quite as much desire to deprecate that setup.

@phosek - What do you think about this? It’s needed to be able to reland D113253.

@phosek - What do you think about this? It’s needed to be able to reland D113253.

Ping @phosek - I'd like to get back on track for relanding D113253 one way or another...

This revision is now accepted and ready to land.Dec 7 2021, 3:57 AM

@AndreyChurbanov - is this ok with you?

AndreyChurbanov accepted this revision.Dec 7 2021, 11:07 AM

@AndreyChurbanov - is this ok with you?

Yes, I am OK with this.