Page MenuHomePhabricator

[CMake] Disallow direct configuration
ClosedPublic

Authored by Hahnfeld on Nov 15 2017, 8:13 AM.

Details

Summary

As a first step, this allows us to generalize the detection of
standalone builds and make it fully compatible when building in
llvm/runtimes/ which automatically sets OPENMP_STANDLONE_BUILD.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Nov 15 2017, 8:13 AM
pirama added inline comments.
CMakeLists.txt
2 ↗(On Diff #123030)

There should be a project() declaration here. If not, CMake adds an empty project() at the top of the file. This breaks the Android build because the setting of the CMP0056 policy via a CMake variable is not obeyed in this case. CMP0056 is needed to correctly configure cross-compile and linking for Android.

Hahnfeld added inline comments.Nov 20 2017, 10:52 AM
CMakeLists.txt
2 ↗(On Diff #123030)

So you're currently configuring runtime/ directly which has a project()?

pirama added inline comments.Nov 20 2017, 10:54 AM
CMakeLists.txt
2 ↗(On Diff #123030)

Yes.

pirama added inline comments.Nov 20 2017, 10:56 AM
CMakeLists.txt
1 ↗(On Diff #123030)

Also, can we bump this up if there are no other issues? The other LLVM projects are all currently using 3.4.3.

Hahnfeld added inline comments.Nov 20 2017, 11:12 AM
CMakeLists.txt
1 ↗(On Diff #123030)

Does that have any benefits? The LLVM requirement was bumped to use new features which I haven't come across yet in the openmp repository. So I'm somewhat reluctant as CMake 2.8 is still the default on CentOS, for example

pirama added inline comments.Nov 20 2017, 11:22 AM
CMakeLists.txt
1 ↗(On Diff #123030)

CMake 3.4.3 defaults to the NEW behavior of CMP0056. That's the only benefit for us, but it's easy enough to work around. I don't know of any other major benefits there might be.

Hahnfeld marked 3 inline comments as done.Nov 20 2017, 11:28 AM
Hahnfeld added inline comments.
CMakeLists.txt
1 ↗(On Diff #123030)

IMO that's not enough to upgrade. If you need that policy, you should submit a patch to enable that one explicitly.

pirama added inline comments.Nov 20 2017, 11:31 AM
CMakeLists.txt
1 ↗(On Diff #123030)

I agree. I'll upload a patch once this current set of changes get submitted.

Hahnfeld updated this revision to Diff 123636.Nov 20 2017, 12:46 PM
Hahnfeld marked 4 inline comments as done.

Add project() to top-level CMakeLists.txt

Hahnfeld updated this revision to Diff 123995.Nov 22 2017, 11:42 AM

Rebase onto recent changes.

More comments on this patch (series)?

jlpeyton added inline comments.Nov 28 2017, 10:05 AM
CMakeLists.txt
15–17 ↗(On Diff #123995)

If it is possible, we should have an OPENMP_ENABLE_RUNTIME option so people can prevent building the runtime/ subdirectory and only build the libomptarget/ directory. Does libomptarget rely on libomp?

jlpeyton added inline comments.Nov 28 2017, 10:10 AM
libomptarget/Build_With_CMake.txt
46–62 ↗(On Diff #123995)

I think libomptarget/Build_With_CMake.txt and runtime/Build_WIth_CMake.txt should be unified and moved into openmp/Build_With_CMake.txt.

grokos added inline comments.Nov 28 2017, 10:17 AM
CMakeLists.txt
15–17 ↗(On Diff #123995)

Yes, it does. It needs functions omp_get_default_device() and __kmpc_omp_taskwait(void *loc_ref, int32_t gtid). More functions may be added to the list of dependencies in the future.

Hahnfeld added inline comments.Nov 28 2017, 10:26 AM
CMakeLists.txt
15–17 ↗(On Diff #123995)

Yes, we could but my viewpoint was that there is no OpenMP with at least the host runtime.

libomptarget/Build_With_CMake.txt
46–62 ↗(On Diff #123995)

Yes, it definitely should. I wanted to tackle this after the renaming of the test settings so that I can put together the file once with the final options.

jlpeyton accepted this revision.Nov 28 2017, 2:42 PM

Ok sounds good. LGTM.

This revision is now accepted and ready to land.Nov 28 2017, 2:42 PM
This revision was automatically updated to reflect the committed changes.