This is an archive of the discontinued LLVM Phabricator instance.

LLVM OpenMP CMake Overhaul
ClosedPublic

Authored by jlpeyton on Jun 23 2015, 9:30 AM.

Details

Summary

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. 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.
  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 system. There are still a few that remain like LibompExports.cmake (copying things to exports/ subdirectory) 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.

Diff Detail

Repository
rL LLVM

Event Timeline

jlpeyton updated this revision to Diff 28248.Jun 23 2015, 9:30 AM
jlpeyton retitled this revision from to LLVM OpenMP CMake Overhaul.
jlpeyton updated this object.
jlpeyton edited the test plan for this revision. (Show Details)
jlpeyton added a reviewer: chandlerc.
jlpeyton set the repository for this revision to rL LLVM.
jlpeyton added subscribers: Unknown Object (MLST), Unknown Object (MLST), cbergstrom, jhowarth.

Is this patch updated to reflect my review comments?

Again some of your design changes remove features that are
intentionally in the existing cmake by design. They should not be
removed. It's essetial that we maintain a mechanism to allow each
compiler the flexibility to define their own flags. Unlike other llvm
projects, this library is very likely to be used standalone by others
entirely outside the scope of llvm.

So do not remove cmake/${CMAKE_C_COMPILER_ID}/*Flags.cmake directories/files

Chris,

I am strongly apposed to this change. This was designed specifically to ensure that each set of compilers can easily set their own specific flags. Do not remove this feature.
Why - who cares? fwiw - most cmake files are named CMakeLists.txt
.cmake is typically used for cmake files which are meant to be reused as "modules".
I am a bit annoyed with the history of the cmake build system. My original system worked and was removed without much justification "just because". Now it's being refactored again and it's unclear to me the driving motivation.

Maybe I can help answer these questions with the aid of the email message below. To get the LLVM OpenMP Runtime set as the default OpenMP runtime library linked when using the -fopenmp flag, we have to get our CMake build to use LLVM conventions/standards. This meant putting everything in its own pseudo namespace Libomp, libomp_ , or LIBOMP_ and creating a real configuration step which checks for compiler flags, features, etc. I've followed what libcxx and compiler-rt do by having a config-ix.cmake file which does all this. Although the compiler flags approach I had worked for gcc,clang,icc,and msvc, this method of checking the compiler flags is more generic and allows any compiler that supports the flag to use it. Also, there are at least three other methods of adding custom flags (-DLIBOMP_${LANG}FLAGS=' ... ' , -DCMAKE_${LANG}_FLAGS=' ... ' , or good ole CFLAGS=' ... ' envirable). The ultimate goal here is to get the LLVM OpenMP Runtime set as the default, so I have to do whatever it takes to achieve that. If I get the ok to keep the old compiler flag method, I will.

  • Johnny
chandlerc requested changes to this revision.Jun 25 2015, 12:22 AM
chandlerc edited edge metadata.

Full pass through this. Thanks again for working on it, this is definitely starting to really improve things.

runtime/cmake/LibompDefinitions.cmake
12 ↗(On Diff #28248)

Rather than this, wouldn't it be better to use a config.h.cmake and process it into a config.h the way LLVM and Clang do? It might even be less code altogether.

runtime/cmake/LibompHandleFlags.cmake
44 ↗(On Diff #28248)

Why do you want to blindly pass -m32 when it is supported? Seems odd that this is the only predicate..

52–55 ↗(On Diff #28248)

Any chance we could just use '-march=native' or something?

58 ↗(On Diff #28248)

This is really concerning. It's an ABI-impacting flag that isn't supported by Clang at all... Not really sure what to do about it though.

runtime/cmake/LibompSourceFiles.cmake
12–13 ↗(On Diff #28248)

This doesn't seem right to me.

We shouldn't have the list of files stored far away from the actual files. The CMakeLists.txt in runtime/src should just pass the source files some function that builds the runtime.

runtime/src/CMakeLists.txt
12–14 ↗(On Diff #28248)

I would somewhat expect the top-level CMake file to handle the inclusion and setup of these facilities, and this one to just use them.

48 ↗(On Diff #28248)

Rather than any support for extracting the date from the build scripts, if this is ever desired, you should just use DATE rather than this.

151–154 ↗(On Diff #28248)

This needs to move to the top so that its clear there are generated files prior to trying to compile them.

chandlerc added inline comments.Jun 25 2015, 12:22 AM
runtime/CMakeLists.txt
30–44 ↗(On Diff #28248)

CMake already provides variables for querying the OS - why define your own?

95–96 ↗(On Diff #28248)

Oh my, I didn't realize that the runtime actually compiled Fortran code.

I think this is pretty bad. It means the particular API exported by the runtime depends on what set of frontends are available. Which in turn means we need folks to acquire a fortran frontend when prepping a build of the runtime to give to others.

I'm not actually a fortran expert, so this may be a naive question, but I thought there was better compatibility between fortran and C and it was possible to define a fortran interface from C... That would certainly be helpful here.

Assuming that's not how it works, and we need to build a fortran component, would it make sense to have it be a separate library name so that it is clear that the C openmp runtime only requires a C compiler to build, and the Fortran bits live in a runtime stacked on top of it and require a Fortran frontend to build?

Probably not for this patch, but something I'd really like to understand.

98–110 ↗(On Diff #28248)

Why provide cache settings for any of these? It seems like unnecessary complexity.

136–137 ↗(On Diff #28248)

Should this really be true? Is this important to support? There isn't much context here about why this is needed.

140–142 ↗(On Diff #28248)

None of the other subprojects do this, only the top-level LLVM project and it sets it to Debug. It seems a bit odd to start doing this and in the other direction. Any particular motivation?

144 ↗(On Diff #28248)

I would suggest creating this below, where you actually need and use it.

146–158 ↗(On Diff #28248)

If you want to do this, it would seem better to do it adjacent to the code that sets up the variables. I would also skip the separate variable for the possible values.

165–170 ↗(On Diff #28248)

At this point we have three different ways to detect Linux vs. Mac. =/ This doesn't seem good at all.

235–239 ↗(On Diff #28248)

I'm curious why these variables are needed?

266–268 ↗(On Diff #28248)

This seems really strange to be a CMake thing and not something in the source code.

294 ↗(On Diff #28248)

As above, I would keep error checking next to the code that sets up the variable.

320 ↗(On Diff #28248)

This seems quite strange. Could you at least leave a comment about why this suffix is needed?

runtime/cmake/LibompCheckLinkerFlag.cmake
14 ↗(On Diff #28248)

Should probably make 'TRUE' and 'false' agree on case.

18–24 ↗(On Diff #28248)

I find the indent here hard to use. Maybe use a more conventional indent?

This revision now requires changes to proceed.Jun 25 2015, 12:22 AM
emaste added a subscriber: emaste.Jun 25 2015, 1:31 PM
emaste added inline comments.
runtime/CMakeLists.txt
30–44 ↗(On Diff #28248)

And FreeBSD is missing if we do define our own; the current convention of lin == Unix-like that is not Apple is confusing.

jhowarth added inline comments.Jun 25 2015, 1:54 PM
runtime/cmake/LibompHandleFlags.cmake
44 ↗(On Diff #28248)

This may be for the building the fat libomp.dyib on x86_64 darwin (containing both i386 and x86_64 code) which requires -m32 to be explicitly passed to the native x86_64 compiler.

chandlerc added inline comments.Jun 26 2015, 1:33 AM
runtime/cmake/LibompHandleFlags.cmake
44 ↗(On Diff #28248)

Sure, I know that this is what -m32 does, but it doesn't make sense to *blindly* pass it. I feel like we should be very specifically testing for targeting i386 with a x86_64 toolchain first.

Oh my, I didn't realize that the runtime actually compiled Fortran code.

I think this is pretty bad. It means the particular API exported by the runtime depends on what set of frontends are available. Which in turn means we need folks to acquire a fortran frontend when prepping a build of the runtime to give to others.

I'm not actually a fortran expert, so this may be a naive question, but I thought there was better compatibility between fortran and C and it was possible to define a fortran interface from C... That would certainly be helpful here.

Assuming that's not how it works, and we need to build a fortran component, would it make sense to have it be a separate library name so that it is clear that the C openmp runtime only requires a C compiler to build, and the Fortran bits live in a runtime stacked on top of it and require a Fortran frontend to build?

Chandler, the issue here is with providing a Fortran module that contains the OpenMP interfaces. Unfortunately, Fortran module files are a binary format file which has no standard specification. therefore to produce a module file for a specific compiler you have to compile the module with that compiler.
The OpenMP standard wants it to be possible for Fortran users to "use omp_lib", that requires that an appropriate Fortran module file exists.

So, the issue is not with the Fortran interface to C functions (the implementation of the Fortran module can use those interfaces if it wants to), but with the fact that Fortran compilers store modules in compiler dependent binary formats. Therefore you need the appropriate Fortran compiler to build the module file.

If a distributor is building the runtime they need to either build the module file on the user's machine at install time (when they can see which Fortran compilers the user has to hand), to maintain a (small) Fortran compiler zoo to build appropriate modules, or explain to the user how to build the module when they install (almost the same as my first solution, but less automatic).

Chandler, the issue here is with providing a Fortran module that contains the OpenMP interfaces. Unfortunately, Fortran module files are a binary format file which has no standard specification. therefore to produce a module file for a specific compiler you have to compile the module with that compiler.
The OpenMP standard wants it to be possible for Fortran users to "use omp_lib", that requires that an appropriate Fortran module file exists.

So, the issue is not with the Fortran interface to C functions (the implementation of the Fortran module can use those interfaces if it wants to), but with the fact that Fortran compilers store modules in compiler dependent binary formats. Therefore you need the appropriate Fortran compiler to build the module file.

If a distributor is building the runtime they need to either build the module file on the user's machine at install time (when they can see which Fortran compilers the user has to hand), to maintain a (small) Fortran compiler zoo to build appropriate modules, or explain to the user how to build the module when they install (almost the same as my first solution, but less automatic).

Wow, thanks for the detailed explanation. I had no idea. =]

So, it seems like this is only of minimal utility to even have in the CMake build, and sadly it brings a reasonable amount of complexity. Is there a reasonable way to separate it (and maybe to make it a script or something that users can easily run to, as you say, build the module for a particular install? Maybe that's the right balance between automatic and usable in different scenarios?

I'm not sure at all, this is a nasty problem relative to the "normal" problems in our runtime libraries.

One of the previous comments in this thread was that there is an
assumption that libomp will be installed to /usr/lib or some system
directory. I absolutely don't believe that should be done. The
-fopenmp or equivalent flag should add the correct (private) includes
(C/C++/Fortran module) and -L/-rpath lib location. This is really
compiler internal and private. LLVM was the odd duck by relying on the
gcc OMP lib before. If llvm had Fortran support - that wouldn't likely
have ever happened from the start.

Also note that -fopenmp=libomp clang doesn't emit the necessary linkage path to find libomp when llvm/clang/compiler-rt/openmp is built with -DCMAKE_INSTALL_PREFIX set to a buried directory like /opt/local/llvm-3.7.

jlpeyton updated this revision to Diff 28583.Jun 26 2015, 11:36 AM
jlpeyton edited edge metadata.

Updating the patch to address Chandler's (and others) comments. I will send another email with responses to questions.

Commented on inline suggestions.

runtime/CMakeLists.txt
30–44 ↗(On Diff #28248)

Good points from both of you. In the second version of this patch I have removed LIBOMP_OS, The build systems relies on the CMake variables WIN32 and APPLE exclusively. I've also added a small section in config-ix.cmake where I define LIBOMP_PERL_SCRIPT_OS based on WIN32 and APPLE. I put a comment there explaining the reasoning behind it and will explain here as well. The perl scripts (which I'm trying to get rid of) have deeply embedded in them an OS check which expects certain OS's. If the user passes --os=lin to the perl script, it checks that lin is valid (which it already is) and will run without problem and use Unix tools under the hood. It is just plain easier and more portable to have Non-Windows and Non-Apple operating systems be considered "lin" in this context (Most unix flavors). This way people on NetBSD for example aren't going to have to dig through the opaque perl module files in runtime/tools/lib and add NetBSD to it.

95–96 ↗(On Diff #28248)

Jim has already given the details on this topic. I haven't addressed any problems with the Fortran Module files in the second iteration, but they aren't created by default (meaning users don't need a Fortran compiler) and shouldn't be a big problem to remove if that is what we decide on.

98–110 ↗(On Diff #28248)

The LIBOMP_MICRO_TESTS is absolutely unnecessary. I've removed the others as well in the second iteration. An argument for the LIBOMP_TEST_* cache variables is if a user is having troubles with one of them, then he/she can just turn it off and not have to think about it again. I have removed them though.

136–137 ↗(On Diff #28248)

I've added a comment and TODO here in the second iteration. The testsuite/ directory expects the library to be placed in the exports/ directory so it can grab it from there. I know this is bad, and I'm trying to get my hands around the testsuite/ to convert it to llvm-lit based testing.

140–142 ↗(On Diff #28248)

I've moved this section under LIBOMP_STANDALONE_BUILD to help reinforce that it is for standalone builds.

OpenMP is often included in HPC codes and I feel the default library should be optimized for performance.
I also feel like most users outside LLVM are going to do this: $ cmake [maybe set compilers] ..
And I want that to build the optimized library.

144 ↗(On Diff #28248)

Done in second iteration.

146–158 ↗(On Diff #28248)

Done in second iteration.

165–170 ↗(On Diff #28248)

This OS detection problem is addressed with my first comment.

235–239 ↗(On Diff #28248)

These are for convenience. They give me absolute source paths no matter what CMakeLists.txt file I'm in or *.cmake file I'm in. The LIBOMP_BASE_DIR variable in particular is popular with most projects because it says "I know exactly where I am. I'm at the very top directory of this project", and then all files can use this variable to navigate.

266–268 ↗(On Diff #28248)

This was requested by ScaleMP. I'm not inclined to remove it since it is off by default.

294 ↗(On Diff #28248)

Done in second iteration.

320 ↗(On Diff #28248)

Well, when this library was first put inside LLVM, we wanted the name to be exactly like Intel's but since that has changed, I have removed this suffix as well. Now Windows will build libomp.dll and libomp.lib.

runtime/cmake/LibompCheckLinkerFlag.cmake
14 ↗(On Diff #28248)

Done in second iteration.

18–24 ↗(On Diff #28248)

I've indented this over four spaces in the second iteration.

runtime/cmake/LibompDefinitions.cmake
12 ↗(On Diff #28248)

Yes, I completely agree. I wanted to tackle this when I remove expand-vars.pl (which is just a perl based configure_file()-like script)

runtime/cmake/LibompHandleFlags.cmake
44 ↗(On Diff #28248)

Compiler-rt blindly passes it in if it detects the i386 symbol and NOT MSVC so I thought it would be ok if we are targeting 32-bit x86.

After looking over libcxx and llvm, I see that they at least do this:
if( CMAKE_SIZEOF_VOID_P EQUAL 8 AND NOT WIN32 )
(logic to include m32)
endif()
Although, they blindly assume the -m32 flag exists (which may not be a huge assumption).
I can add this wrapper in the third iteration.

52–55 ↗(On Diff #28248)

I've removed the checks for /arch:SSE and /arch:IA32 and -msse and kept the msse2 check. The second iteration now will just append msse2 if they have it. -march=native should be something the user specifies.

58 ↗(On Diff #28248)

Here is some info about it: https://software.intel.com/sites/products/documentation/doclib/iss/2013/compiler/cpp-lin/GUID-8EB32E6C-EA4B-4D2A-B4BF-9FA2774C505A.htm
It is an Intel compiler specific flag that was inserted to maintain binary compatibility when GCC changed their ABI on 32-bit x86 which started making the assumption that stacks were 16 byte aligned to aid SSE instructions. It is supposed to be the most backwards compatible flag.

runtime/cmake/LibompSourceFiles.cmake
12–13 ↗(On Diff #28248)

Can I just move this logic to runtime/src/CMakeLists.txt and delete LibompSourceFiles?

runtime/src/CMakeLists.txt
12–14 ↗(On Diff #28248)

Moved to top-level CMakeLists.txt file in second iteration.

48 ↗(On Diff #28248)

I'll have to change this in the third iteration. FWIW, CMake offers string(TIMESTAMP ...) in v2.8.11 so someone wanted it :)

151–154 ↗(On Diff #28248)

Moved to top in second iteration.

jlpeyton updated this revision to Diff 28812.Jun 30 2015, 2:07 PM
jlpeyton edited edge metadata.

Addressing most of Chandler's remaining issues with this third iteration.

  • There is no longer a LibompSourceFiles.cmake file. The files are grabbed inside src/CMakeLists.txt
  • The -m32 flag is guarded by if(CMAKE_SIZEOF_VOID_P EQUAL 8) so we only use it when using a x86_64 toolchain to compile i386.
  • Removed passing in of _KMP_BUILD_TIME.
  • I've gotten the library to build using Unix Makefiles, NMake Makefiles, and NInja on Windows in standalone mode. There is still one issue when trying to build in a Windows LLVM tree which is that LLVM adds numerous -wXYZ flags by using add_llvm_definitions() which causes the resource compiler (RC.exe) to include them in its command which causes an unknown flag error.

FYI, I have built current openmp trunk with the the third permutation of the cmake overhaul applied as part of an in-tree llvm/clang/compiler-rt/libc++/polly cmake build using a 3-stage bootstrap with stage2/stage3 comparison of binaries on x86_64-apple-darwin13/14/15. The only minor nit I found was that three files should differences between stage2/stage3 due to the timestamps added by

-- stage2/projects/openmp/runtime/src/kmp_i18n_default.inc	2015-07-04 17:04:13.000000000 -0400
+++ stage3/projects/openmp/runtime/src/kmp_i18n_default.inc	2015-07-04 17:28:37.000000000 -0400
@@ -1,5 +1,5 @@
 // Do not edit this file! //
-// The file was generated from en_US.txt by message-converter.pl on Sat Jul  4 17:04:13 2015. //
+// The file was generated from en_US.txt by message-converter.pl on Sat Jul  4 17:28:37 2015. //
 
 static char const *
--- stage2/projects/openmp/runtime/src/kmp_i18n_id.inc	2015-07-04 17:04:13.000000000 -0400
+++ stage3/projects/openmp/runtime/src/kmp_i18n_id.inc	2015-07-04 17:28:37.000000000 -0400
@@ -1,5 +1,5 @@
 // Do not edit this file! //
-// The file was generated from en_US.txt by message-converter.pl on Sat Jul  4 17:04:13 2015. //
+// The file was generated from en_US.txt by message-converter.pl on Sat Jul  4 17:28:37 2015. //
 
 enum kmp_i18n_id {

and projects/openmp/runtime/src/CMakeFiles/omp.dir/kmp_version.c.o which I assume is related to those two diffs. Would it be possible to modify message-converter.pl so that it doesn't emit the time-stamp?

I feel there has been ample opportunity to review the new iterations of this patch. And if no one objects, I'm going to commit this tomorrow. Post commit reviews are always welcome.

  • Johnny

I can get rid of the timestamps easily in the kmp_i18_n* files and the kmp_version.c file (which uses TIME and DATE macros now) can probably be easily deleted as well but involves changing the source code a little. I will do this in a different patch though as I want this patch to be exclusively CMake.

  • Johnny

Chandler,
I apologize. You’re absolutely right.

Jack,
My ETA for when I plan to commit this patch to trunk is when it gets approved.

  • Johnny

From: Chandler Carruth [mailto:chandlerc@google.com]
Sent: Tuesday, July 7, 2015 11:00 AM
To: Peyton, Jonathan L; Jack Howarth; reviews+D10656+public+dda1fdcafb4c32bf@reviews.llvm.org
Cc: C Bergström; openmp-commits@dcs-maillist2.engr.illinois.edu; howarth.mailing.lists@apple.com; llvm-commits@cs.uiuc.edu
Subject: Re: [Openmp-commits] [PATCH] LLVM OpenMP CMake Overhaul

Jonathan, that is not the code review policy of the LLVM community. Please, do not try to force code review by setting a time table. Once you ask for code review, it is your responsibility to wait for your reviewers to approve the patch.

I'm sorry that last week didn't get you a code review. Lots of folks were on vacation, and in the US it was a short week due to holidays. Unfortunately, this is the reality, and doesn't mean you should rush the patch.

emaste added a comment.Jul 7 2015, 9:45 AM

Git complained when I applied this locally:

<stdin>:347: trailing whitespace.
set(LIBOMP_TOOLS_DIR ${LIBOMP_BASE_DIR}/tools) 
<stdin>:472: trailing whitespace.
# - stats-gathering enables OpenMP stats where things like the number of 
<stdin>:2076: trailing whitespace.
         add_library(foo SHARED src_to_link.c)") 
<stdin>:2146: trailing whitespace.
    
<stdin>:2149: space before tab in indent.
        libomp_append(cppflags_local "-D CACHE_LINE=128")
warning: squelched 43 whitespace errors
warning: 48 lines add whitespace errors.

Applying this on top of git commit bb405b5 in a clean source tree, I don't have any problems.

$ patch -p0 < D10656.diff
patching file runtime/CMakeLists.txt
patching file runtime/cmake/BuildPLRules.cmake
patching file runtime/cmake/Clang/AsmFlags.cmake
...

  • Johnny

No problem here applying...

http://reviews.llvm.org/file/data/opiipyz7zmrkghbkwlex/PHID-FILE-oa2uwwoaeemkrlkuc7ll/D10656.id28812.diff

for diff3 ID 28812 onto openmp at r241603 for an svn pull.

emaste added inline comments.Jul 7 2015, 10:02 AM
runtime/cmake/LibompMicroTests.cmake
18 ↗(On Diff #28812)

+FreeBSD

23 ↗(On Diff #28812)

+FreeBSD

28 ↗(On Diff #28812)

+FreeBSD

feynman% ninja libomp-test-execstack   
[1/1] Generating test-execstack/.success
33 ↗(On Diff #28812)

Note that the failure message when I try to run it seems to contradict this statement:

check-instruction-set.pl: (x) Only works on lin_32 and lin_mic platforms.
38 ↗(On Diff #28812)

+FreeBSD

[1/1] Generating test-deps/.success
check-depends.pl: (i) Dependencies:
check-depends.pl: (i)     libc.so.7
check-depends.pl: (i)     libthr.so.3
runtime/src/CMakeLists.txt
104–106 ↗(On Diff #28812)

Nitpick, should these be z_Unix_util.c and z_Unix_asm.s?

Applying this on top of git commit bb405b5 in a clean source tree, I don't have any problems.

Perhaps git doesn't have the warning on by default and I enabled it in my config; in any case I tagged a couple lines above with an example of the complaints. Note that it's a warning only, the patch still applies.

runtime/CMakeLists.txt
192 ↗(On Diff #28812)

EOL whitespace here, for example

242 ↗(On Diff #28812)

EOL whitespace here

runtime/cmake/LibompCheckLinkerFlag.cmake
24 ↗(On Diff #28812)

EOL whitespace

jlpeyton updated this revision to Diff 29209.Jul 7 2015, 2:12 PM
  • In LibompMicroTests.cmake comments, changed the Linux references to Unix so as not to suggest only Linux support, but we should not try to keep track of every OS.
  • Added comment that the test-instr microtest is available for the i386 architecture.
  • Removed libunwind.so from test-deps microtest for FreeBSD.
  • Eliminated trailing whitespaces in all files.

The r241832 and r241833 commits seem to have spoiled the context for applying the proposed cmake overhaul patch.

jlpeyton updated this revision to Diff 29376.Jul 9 2015, 1:08 PM
jlpeyton removed rL LLVM as the repository for this revision.

Updating patch to be applied on top of 7f88ff4 (git) or r241833 (svn). Basically had to merge the recently added debugger CMake code into this patch.

So are we close to completion with the review process or is there more bikeshedding to be done?

jlpeyton updated this revision to Diff 29380.Jul 9 2015, 1:22 PM
jlpeyton set the repository for this revision to rL LLVM.

Changed debugger code to be off by default and set the Repository back to LLVM. Sorry for the inconvenience.

runtime/CMakeLists.txt in 29380 doesn't apply cleanly to current openmp svn...

patching file runtime/CMakeLists.txt
Hunk #1 FAILED at 9.
1 out of 5 hunks FAILED -- saving rejects to file runtime/CMakeLists.txt.rej

jlpeyton updated this revision to Diff 29395.Jul 9 2015, 2:42 PM

Fixing patch issue brought up by Jack Howarth.

If this misses the branching of 3.7.0, it should be committed to both trunk and 3.7.0 branch.

chandlerc accepted this revision.Jul 14 2015, 7:08 PM
chandlerc edited edge metadata.

First and foremost, thank you again for working on this.

I want to call out one thing that was very helpful in case it went unnoticed. Your replies of things that you're planning to do in subsequent iterations and your replies giving context as to *why* things are the way they are. Totally useful.

Also, I appreciate the added comments archiving context for the next reader. I don't want you to feel like that was wasted time, it helps tremendously.

At this point, I think you should commit this after addressing the remaining (minor) comments from Ed Maste and some of the simple things below. This is *significantly* better than the starting point, and I'm interested in letting you get to the next iteration.

Once it lands, I'll also test out using it, and look to see if I spot any actual functional issues.

runtime/CMakeLists.txt
34 ↗(On Diff #29395)

Maybe just convert everything to 2-space indent? That would be most consistent with the rest of LLVM, but maybe not the openmp runtime. But within the else() side of this if() you have 2-space indent creeping in already. I think having them be consistent will be really useful for readability.

196 ↗(On Diff #29395)

This comment seems now out of place.

268–274 ↗(On Diff #29395)

A question that isn't relevant for this patch, but I didn't want to forget.

Would it make sense to use '_prof' and '_stubs' so the names read a bit easier? Or are there users relying on these suffixes? Mostly curious.

This revision is now accepted and ready to land.Jul 14 2015, 7:08 PM
This revision was automatically updated to reflect the committed changes.

Commit rL242298 still needs to be applied to the openmp 3.7.0 branch as well.

Addressing Ed Maste's comments directly in Phabricator (they were already addressed silently in the code)

First and foremost, thank you again for working on this.

I want to call out one thing that was very helpful in case it went unnoticed. Your replies of things that you're planning to do in subsequent iterations and your replies giving context as to *why* things are the way they are. Totally useful.

Also, I appreciate the added comments archiving context for the next reader. I don't want you to feel like that was wasted time, it helps tremendously.

At this point, I think you should commit this after addressing the remaining (minor) comments from Ed Maste and some of the simple things below. This is *significantly* better than the starting point, and I'm interested in letting you get to the next iteration.

Once it lands, I'll also test out using it, and look to see if I spot any actual functional issues.

Chandler,

Thanks for reviewing this patch. I know it wasn't the ideal in terms of size, but needed to be done. I appreciate your feedback and direction.

I apologize but didn't see the recommendation to change the indention to two space indention. So I *QUICKLY* committed a follow up patch which addressed your comments. I changed the indention (which I understand makes an awful patch) and moved the comment about enabling MASM. Regarding the prof and stubs library, the prof library is something I need to remove, and I'm not opposed to changing the stubs library name. I'm not sure anyone has actually built the stubs library except for me... :) . And in case anyone was wondering what the stubs library is for, it is a minimal, serial implementation of the OpenMP runtime. For example, omp_get_thread_num() will always return 0 in the stubs library. When someone doesn't want to compile there OpenMP code with -fopenmp, but needs the omp_* API, they can link to the stubs library.

Also thanks to Jack Howarth, Ed Maste, Chris Bergstrom, Jim Cownie, for leaving feeback, and testing the patch out.

runtime/cmake/LibompMicroTests.cmake
19 ↗(On Diff #29395)

I've changed all these to just Unix so we don't have to enumerate all the Unix flavors.

34 ↗(On Diff #29395)

I've added to the comment that i386 architecture is supported as well.

39 ↗(On Diff #29395)

I've removed the libunwind library from FreeBSD's list.

openmp/trunk/runtime/cmake/PerlFlags.cmake