[Openmp-dev] CMake overhaul

[Peyton, Jonathan L]

For everyone who wants a Phabricator review and is not on openmp-commits mailing list.

http://reviews.llvm.org/D10656

-- Johnny

From: openmp-dev-bounces at cs.uiuc.edu [mailto:openmp-dev-bounces at cs.uiuc.edu] On Behalf Of Peyton, Jonathan L
Sent: Tuesday, June 23, 2015 11:16 AM
To: Chandler Carruth; openmp-dev at dcs-maillist2.engr.illinois.edu
Subject: Re: [Openmp-dev] CMake overhaul

> One question, would it be OK to use Phabricator for the review?
Absolutely.

> Have you looked much at the libc++ and/or compiler-rt CMake build?
Yes, Its where I got the idea for LIBOMP_LIBDIR_SUFFIX (libc++) and config-ix.cmake.  I’ve looked at the compiler-rt CMake build system, and it seems like more than we need.  It creates multiple libraries that have the architecture in the name (something like libclang_rt.msan-x86_64.a).  I only want to create one libomp.so file so libc++ seemed the better candidate to look at.  I’m still sifting through libc++ and the LLVM cmake build system to see if there are more predefined LLVM_* CMake variables I can use.

In terms of configuration, libomp is based off an old code base and needs more feature checking, which I still need to add, than libc++ which is written in high level C++.  I’m not even positive that libc++ changes much based on architecture.  Looking more at the structure of the libcxx and compiler-rt cmake/ directory, I could move every Libomp*.cmake file into libomp/cmake/Modules/ subdirectory similar to the other runtime libraries.

> My final high level question (which probably just reflects my not knowing the current status) is what the progress is on using the lit test infrastructure?
The progress here is that it is something else I’ll have to rip apart and add, but it needs to be done.  First, I just wanted the base CMake build system to be Ok.
If I can ask a few questions myself, When you say lit test infrastructure, you mean having a check-libomp target that will call llvm-lit directly inside the testsuite directory with proper lit flags, correct?
Also, these tests should have RUN: and CHECK: lines in them as per http://llvm.org/docs/TestingGuide.html ?

> So, looking at the source code layout, what is the plan around the offload tree?
I think Sergey Ostanevich knows better about this.  I know that the offload/ subdirectory is Intel® MIC Architecture specific and the build of it requires one to build a “host” version which targets the platform hosting the MIC card and a “target” version which targets the Intel® MIC Architecture.

> I think it might be worth discussing with the broader community if there is any precedent here, especially on other OSes. There might be others in the community with suggestions about how this *should* work. It's really hard for me to make suggestions because currently the comments in the BuildPLRules.cmake (haven't dug into your new patch fully yet) are in terms of ICC and compiling products that aren't in LLVM's upstream repository at all.
Are you talking about the config-ix.cmake file in your first couple sentences here?  BuildPLRules.cmake was a file created solely to imitate the exact build process of the original build.pl+Makefile build system.  It currently doesn’t work right now.

-- Johnny

From: Chandler Carruth [mailto:chandlerc at google.com]
Sent: Tuesday, June 23, 2015 1:39 AM
To: Peyton, Jonathan L; openmp-dev at dcs-maillist2.engr.illinois.edu<mailto:openmp-dev at dcs-maillist2.engr.illinois.edu>
Subject: Re: CMake overhaul

On Mon, Jun 22, 2015 at 2:26 PM Peyton, Jonathan L <jonathan.l.peyton at intel.com<mailto:jonathan.l.peyton at intel.com>> wrote:
Hello everyone,
I have been working on overhauling the CMake build system for the LLVM OpenMP Runtime library.  Instead of sending 1000 small updates, I decided to do a complete refactoring/reorganizing/renaming/you-name-it-I-did-it.  **All the interface variables are still the same (LIBOMP_OS, LIBOMP_ARCH, etc.)**.  So there was no change there.

FWIW, I'm excited to see this work.

One question, would it be OK to use Phabricator for the review? While I agree and plan on patching and looking at the patch directly, creating comments on specific parts of the change is *much* easier for me with Phabricator, so I'll be able to review it there faster.

Have you looked much at the libc++ and/or compiler-rt CMake build? I've suggested this a few times before, but it may provide a useful reference for some simplifying concepts.

Relatedly, I think it would be good to discuss this with Chris Bieneman who has been working on the compiler-rt CMake build. Specifically, I don't know what the host compiler requirements are for the runtime, but some of our runtimes require that they are built with the just-built-Clang compiler.

My final high level question (which probably just reflects my not knowing the current status) is what the progress is on using the lit test infrastructure? I don't want to add a bunch of comments about that here if it really needs to be dealt with separately, but I suspect one of the key issues is going to be integrating the cmake build with the lit test infrastructure and how that works.
Here are the major changes:
1) Renaming internal variables/macros/functions – everything has libomp_ or LIBOMP_ prefixed to it unless maybe if it is a local variable/parameter inside a function/macro.
So, looking at the source code layout, what is the plan around the offload tree? I find the structure here confusing.

We have a 'runtime' directory, and an 'offload' directory. But they're both documented as being runtime libraries... Is there any plan to have non-runtime-library code here? My strong impression was "no", but you certainly would have more context! =]

If this is all going to be part of a collection of runtime libraries, and the offload stuff just hasn't been fully integrated, I would like to make a suggestion (which has some relevance here, but we might want to discuss it elsewhere in more detail) that you (or someone) re-organize the directories along the lines of:

.../openmp/runtime/... -> .../openmp/...
.../openmp/offload/src/... -> .../openmp/src/offload/...
.../openmp/offload/doc/... -> .../openmp/doc/offload/...
.../openmp/offload/README.txt -> .../openmp/src/offload/README.txt

Would that make sense? As a relative newcomer to the library, it makes more sense to me,and so it might be a bit better at pulling people in, but I dunno if it just doesn't fit the larger organization.
2) config-ix.cmake added – compiler flag checks, linker flag checks, some feature checks, threading-model check.  This means cmake/${CMAKE_C_COMPILER_ID}/*Flags.cmake directories/files are deleted.
3) All files inside cmake/ subdirectory are renamed to Libompxyz.cmake
4) I cut off most of the vestigial parts that were a part of the translation of build.pl<http://build.pl> system.  There are still a few that remain like LibompExports.cmake (copying things to exports/ subdirectory)
I think it might be worth discussing with the broader community if there is any precedent here, especially on other OSes. There might be others in the community with suggestions about how this *should* work. It's really hard for me to make suggestions because currently the comments in the BuildPLRules.cmake (haven't dug into your new patch fully yet) are in terms of ICC and compiling products that aren't in LLVM's upstream repository at all.

If this is solely to support building out-of-tree products around the runtime, I think you may end up needing to pull specific bits of it up into your out-of-tree code, as it doesn't make a lot of sense as is for the upstream folks.
and LibompMicroTests.cmake (small tests to run on the just created library.  This is invoked with make libomp-micro-tests), but these can easily be cut off because they are in their own files and surrounded by an if() guard in the CMakeLists.txt file.
5) Added LLVM-architecture detection if we are part of an LLVM build.
6) If you want, you can use LIBOMP_ARCH=i386 or LIBOMP_ARCH=x86_64 instead of LIBOMP_ARCH=32 or LIBOMP_ARCH=32e (although 32 and 32e still exist)
7) Unused functions/macros have been cut away.
Again, thanks for working on this. I'll try to dig into the new cmake code itself next.

This is easier to review if you just patch and take a look at files runtime/CMakeLists.txt, runtime/src/CMakeLists.txt, cmake/Libomp*.cmake:
$ patch –p1 –E < CMake_overhaul_v1.patch
[-E removes empty files created by the patch]

I’ve tested this out on x86 Linux, Mac, MIC, Windows and it works, but I can’t test it on other architectures.

Leave feedback, recommendations.

-- Johnny
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/openmp-dev/attachments/20150623/cd2fcbdd/attachment.html>