Initial cmake support for openmp patch with CMAKE_CXX_COMPILER hack removed as requested. Tested on x86_64-apple-darwin12 against the system clang compilers from Xcode 5.1.1 and on x86_64 Fedora 15 against the system gcc 4.6.3 compilers.
initial_cmake_support_v3.patch (5.42 KB)
Jack,
Please review this patch with Jim Cownie – as he maintains libiomp and obviously, should be involved.
Andrey
Initial cmake support for openmp patch with CMAKE_CXX_COMPILER hack removed as requested. Tested on x86_64-apple-darwin12 against the system clang compilers from Xcode 5.1.1 and on x86_64 Fedora 15 against the system gcc 4.6.3 compilers.
LGTM. Landed in r209994!
Note that I've split out a CMakeLists.txt that includes the one in src, and updated the documentation in README.txt explaining that the CMake system is experimental and not supported for production builds.
There's still some work to get this up and running on Windows, FreeBSD, icc, and the configuration needs to be made more flexible but it's a great start. I'd also like to check in with Jim to make sure this works with the Intel setup at some point.
Thanks Jack and C. Bergström for the initial contribution.
Alp.
Sorry, I am going to revert this check in.
* You are moving too fast; all of this has been done since noon on Friday here in the UK,
and I was out on Friday afternoon and do not work at weekends. I have therefore had no chance to
look at it before it was checked in. (And I'm supposed to be a key reviewer and architect here...)
* A change like this (which provides a whole new build system) requires more than one review.
* Since everyone complained so much, we have been working on a CMAKE based build system here at Intel
that we hope to push this week, which *does* support Windows, icc, gcc, clang etc
So, at the point when I commit that, I'm going to remove these changes.
By all means give me grief and complain about your wasted time. There's not a lot I can do about it, though.
-- Jim
James Cownie <james.h.cownie@intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438
At the point (1 week?) Intel or someone else proposes a new build system which has been reviewed and accepted. It makes total sense to remove what's there already. Adding a new build system doesn't make the sky fall. Please don't be upset because you didn't have a chance to review the changes. It's not rocket science and has been tested by more than one person with expert knowledge.
Wasted effort or not - I'm happy someone cared enough to take this matter on and do it.
Hi Jim,
I think there are a few things seriously wrong here.
* You are moving too fast; all of this has been done since noon on Friday here in the UK,
and I was out on Friday afternoon and do not work at weekends. I have therefore had no chance to
look at it before it was checked in. (And I'm supposed to be a key reviewer and architect here...)* A change like this (which provides a whole new build system) requires more than one review.
In open source, incremental changes are usually preferred. Adding a new (not-yet-default) built system in response to community need is a good thing, even if it is not perfect.
* Since everyone complained so much, we have been working on a CMAKE based build system here at Intel
that we hope to push this week, which *does* support Windows, icc, gcc, clang etc
That is simply not how open source development works. If you have a private fork that has extensive changes and someone commits something that makes merging them difficult then that is *your problem*, not that of the wider community. This is the fundamental basis of open source development: stuff in the public repository is canonical.
Developing code in private and then doing big code drops is *not* a good way of interacting with the community. With my hat on as a representative of an operating system that ships clang as the default compiler and would like to see OpenMP work out of the box, I've been following this list and I was not aware that this is something that you were working on.
Discussions about incorporating a CMake build system date back to 3 March on this list, and I did not find any posts from anyone at Intel indicating that you guys were working on it until your post from today. Indeed, I identified lack of a functional build system as one of the key blockers to portability in my first email to this list.
So, at the point when I commit that, I'm going to remove these changes.
I trust that your changes will undergo external review?
David
Hi Jim,
I think there are a few things seriously wrong here.
* You are moving too fast; all of this has been done since noon on Friday here in the UK,
and I was out on Friday afternoon and do not work at weekends. I have therefore had no chance to
look at it before it was checked in. (And I'm supposed to be a key reviewer and architect here...)* A change like this (which provides a whole new build system) requires more than one review.
In open source, incremental changes are usually preferred. Adding a new (not-yet-default) built system in response to community need is a good thing, even if it is not perfect.
* Since everyone complained so much, we have been working on a CMAKE based build system here at Intel
that we hope to push this week, which *does* support Windows, icc, gcc, clang etc
That is simply not how open source development works. If you have a private fork that has extensive changes and someone commits something that makes merging them difficult then that is *your problem*, not that of the wider community. This is the fundamental basis of open source development: stuff in the public repository is canonical.
Developing code in private and then doing big code drops is *not* a good way of interacting with the community. With my hat on as a representative of an operating system that ships clang as the default compiler and would like to see OpenMP work out of the box, I've been following this list and I was not aware that this is something that you were working on.
Discussions about incorporating a CMake build system date back to 3 March on this list, and I did not find any posts from anyone at Intel indicating that you guys were working on it until your post from today. Indeed, I identified lack of a functional build system as one of the key blockers to portability in my first email to this list.
So, at the point when I commit that, I'm going to remove these changes.
I trust that your changes will undergo external review?
David
OK, sorry, my tone may have been wrong here.
I am absolutely glad to have people contributing, and certainly don't want to put people off.
(ISTR that I bought Alp at least one beer when we met in London :-)).
-- Jim
James Cownie <james.h.cownie@intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438
Hi David,
Discussions about incorporating a CMake build system date back to 3 March on this list, and I did not find any posts from anyone at Intel indicating that you guys were working on it until your post from today
I did mention this: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-May/037218.html
Also, I explicitly asked to have code reviewed by Jim before committing Jack’s patch: http://lists.cs.uiuc.edu/pipermail/openmp-dev/2014-June/000146.html. Unfortunately, this was ignored.
Re-making build system without even letting project’s architect to do code review is simply… wrong, don’t you think so?
Yours,
Andrey
James,
Sorry about that, I assumed that you had signed off on those commits off-list. Concerning the CMakeLists.txt files from https://github.com/pathscale/openmprtl/tree/master/itt/libomp_oss, I tried with those originally but they don’t work without additional changes. They also so not produce the same exact build as the ‘make compiler=clang’ build method. My modified files are a complete reconstruction of their functionality with care taken on both darwin and linux to reproduce the exact behavior of build.pl under the cmake system.
Jack
ps The major changes in my implementation of the cmake files compared to those at openmprtl are…
if(APPLE)
set(CMAKE_SHARED_LINKER_FLAGS “${CMAKE_SHARED_LINKER_FLAGS} -current_version 5.0”)
set(CMAKE_SHARED_LINKER_FLAGS “${CMAKE_SHARED_LINKER_FLAGS} -compatibility_version 5.0”)
endif()
if (NOT APPLE)
set(FEATURE_FLAGS “${FEATURE_FLAGS} -D KMP_TDATA_GTID”)
endif()
to make the cmake build on linux match the build.pl one.
set(FEATURE_FLAGS “${FEATURE_FLAGS} -D INTEL_ITTNOTIFY_PREFIX=_kmp_itt”)
set(FEATURE_FLAGS “${FEATURE_FLAGS} -D USE_ITT_NOTIFY=1”)
set(FEATURE_FLAGS “${FEATURE_FLAGS} -D INTEL_ITTNOTIFY_PREFIX=_kmp_itt”)
to make the cmake build on linux and darwin match the one using build.pl. Windows is untested of course.
Addition of kmp_cancel.cpp and kmp_itt.c to SOURCES file list to match the build from build.pl on darwin and linux.
Removal of kmp_ftn_stdcalls.c from the SOURCES file list as it is not used in the build from build.pl on either darwin or linux.
Addition of the missing “-D KMP_ASM_INTRINS” to the add_custom_command for generating z_Linux_asm.o so that behavior of build.pl is emulated in cmake. I did this because I noticed that z_Linux_asm.s contained a preprocessor testing this variable.
Implemented the use of…
set(OMP_SHLIBEXT “${CMAKE_SHARED_LIBRARY_SUFFIX}”)
to handle the shared library suffix in a generic manner.
set(FEATURE_FLAGS “${FEATURE_FLAGS} -D KMP_USE_ADAPTIVE_LOCKS=1”)
set(FEATURE_FLAGS “${FEATURE_FLAGS} -D KMP_DEBUG_ADAPTIVE_LOCKS=0”)
for darwin and linux to match the behavior of build.pl.
set(COMMON_FLAGS “${COMMON_FLAGS} -Wno-unused-value”)
set(COMMON_FLAGS “${COMMON_FLAGS} -Wno-switch”)
set(COMMON_FLAGS “${COMMON_FLAGS} -Wno-deprecated-register”)
to match the warning flags emitted for “make compiler=clang” by build.pl on darwin and linux.
OK, sorry, my tone may have been wrong here.
I am absolutely glad to have people contributing, and certainly don't want to put people off.
Hi Jim,
There's no problem at all in superseding the current system if, as it sounds, the one you've been cooking up is an improvement! I'm excited but it's the first anyone in the community has heard of this.
It should be made clear that the current OpenMP runtime CMake build system has been in development for some time, including on-list discussions in the LLVM community that go back weeks following all the best practices we have. The only thing that changed is that C. Bergstrom graciously provided the sign-off we needed to integrate Jack's work late last week.
So it's a mischaracterisation to say this happened over the weekend. Even if it did that would be on the long side compared to timescales seen on llvm-commits. Thanks to Jack Howarth and C. Bergström we now have a pretty cool build system in ToT which serves users who were previously excluded due to the custom build scripts, while there's still no public patch available for the work you mentioned this morning.
In general it's a good idea to participate in on-list discussions and give a heads up if you see people discussing features you have plans for. Is there anything else in the pipeline?
(ISTR that I bought Alp at least one beer when we met in London :-)).
It'll take more than one beer! Seriously though, I'm sure the two pieces of work will converge trivially. The next round's on me so please lighten up 
Cheers
Alp.
Alp,
With all respect, a few of assertions you made are simply not true.
Andrey,
Reading through the the thread at http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140519/106158.html, I can understand the sensitivities here on the topic of reviews. IMHO, the process of integrating clang-omp and openmp into the standard llvm/compiler-rt/clang build would go much smoother if the merge of clang-omp changes were sent up stream as a cohesive set of patches to merge the branch like FSF gcc does. I know this will set the hair on edge for some of the llvm developers, but when a merge is submitted as a single set of patches, the upstream developers are forced to take the review process far more seriously. Especially, if the reviews are coming in slowly, submitting these patches upstream in a piecemeal approach will only aggravate the problem of timely reviews.
Jack
Andrey,
Also, note that when I say a single set of patches, I don’t mean a single patch but number individual patches submitted as a complete patch set. After many years of carefully monitoring merges in FSF gcc (and helping mitigate the breakage from them on the darwin targets since Apple abandoned gcc), it has become clear that there are certain social pressures in the review process that a unified patch set creates. When a complete set of patches are submitted and say 90% of them are quickly reviewed, approved and committed, this results in a social pressure for the remaining reviewers to accelerate their work so as to not be seen as retarding the merge.
Jack
Alp,
With all respect, a few of assertions you made are simply *not true*.
It should be made clear that the current OpenMP runtime CMake
build system has been in development for some time, including
on-list discussions in the LLVM community that go back weeks
following all the best practices we have. The only thing that
changed is that C. Bergstrom graciously provided the sign-off we
needed to integrate Jack's work late last week.What "discussions... that go back weeks" you are speaking about?!
In this case we have had a review period spanning three days, with feedback from three developers including an *OpenMP expert* (and final review from technically the *top committer* on the LLVM openmp module given that the "code drop" commit was monolithic), following *three iterations* of the patch posted to the list for public scrutiny and amendments. This is unequivocally a *good thing*.
If you doubt that, take a look at the previous non-trivial commits and "code dumps" -- now tell me, which sounds like best practice between the two?
Jack started his "On Improving the Build System revisited" thread on May 30. This is four days ago, not weeks.
The plan to introduce the CMake build system originated months ago and the original code was written before then, but not signed off until last week.
This has been coordinated in full openness.
And since when "all the best practices" include introducing a new build system without getting project architect's consent? -- especially after explicitly asked to do so, a message that you conveniently ignored.
So it's a mischaracterisation to say this happened over the
weekend. Even if it did that would be on the long side compared to
timescales seen on llvm-commits.What timescales you are speaking about?!
Reviews on cfe-commits frequently happen faster than the *15 minute* mailing list delay which causes commits to arrive before the patch proposal. Not always great but we deal with it. You are actually free to suggest and provide improvements after a change has landed.
For reference, we wait for *weeks* for our OpenMP in clang patches to be reviewed! And we commit them *only* after explicit consent of one of clang code owners -- even if we already got code review from someone else.
And we're trying to improve that workflow. Who wants to wait for *weeks* when we have qualified reviewers?
In general it's a good idea to participate in on-list discussions
and give a heads up if you see people discussing features you have
plans for. Is there anything else in the pipeline?That's *exactly* what we did back in March.
http://lists.cs.uiuc.edu/pipermail/openmp-dev/2014-March/000055.html
You asked about "Cmake generating gmake makefiles?" in that mail and the community has responded.
Also note that the one commit below is all we actually know about your work and your relation is to the project -- your mail was arrogant enough that I wouldn't have responded were it not for your association with Jim Cownie who we know and work with, and the unfortunate attitude towards people who have helped the project. Note that I've offered to give my own time to help Jim merge your CMake changes with the system in ToT because it's a nice thing to do.
$ git log --author=Andrey
commit c88ab54e0d3d89c97175d21d6af3466df5445eaa
Author: Andrey Churbanov <Andrey.Churbanov@intel.com>
typo fixed as a test commit
Alp.
Alp,
Its obvious we have started off on the wrong foot here. Please lets not fixate on any objections that you had to my initial posts and try to have a productive interaction here. As far as I know, everyone here has the same goal of seeing clang-omp/openmp land in an llvm release as soon as feasible. I am just relating what I have observed of the social dynamics of the review process from over on FSF gcc. While it is a different system, it still is the same sort of pool of varied personalities with the same random conflicts (as by testified by how poorly we have hit it off).
Jack
Hello All,
I have been here at Intel working on an ‘exact’ translation of the build.pl build system to an identical CMake build system (without the build.pl Perl wraparound of course). I’ve looked at the recently added CMake build system and appreciate the work you all have done. The system
I’ve created has Windows support, Fortran support, Mac Fat Library support, Intel-specific header creation support, Intel MIC support as well as mirroring the build.pl system for both Mac and Linux using clang, gcc, or icc.
All build types (release, debug) and library types (stubs, normal) are supported as well. This build system is currently going through the review process and should be done very soon.
I’d be happy to answer any questions regarding the new CMake build system.
Thanks,
Johnny
Jack,
The approach we (Intel) use is standard for clang community. No changes in how we prepare / submit patches will speed-up code review process. Limiting factor here is small number of clang code owners / their limited throughput (which is understandable given how many things they carry on their shoulders).
We tried to extend number of code reviewers for our patches, but as the mail thread you linked tells, not many people has enough authority to approve patches for clang. At least yet – hopefully, this will change in the future.
There is hope, though – Richard Smith, who is one of clang code owners, approved us to commit Sema (parsing + semantic analysis) parts of our patches in “review after commit” fashion. Given that these pathces comprise 50% of all code, this should speed up upstreaming a lot.
Yours,
Andrey
The Perl build system makes it very hard to bring up a new system, as it uses targets (rather than features) in a lot of places (and has several different notions of what the target is in different places). I hope that any changes that you make to the CMake build system will not make this harder.
David
From: "Andrey Bokhanko" <andreybokhanko@gmail.com>
To: "Alp Toker" <alp@nuanti.com>
Cc: openmp-dev@dcs-maillist2.engr.illinois.edu, "David Chisnall" <dc552@cam.ac.uk>
Sent: Monday, June 2, 2014 10:17:50 AM
Subject: Re: [Openmp-dev] [PATCH] [Revisedx2] Initial cmake supportAlp,
With all respect, a few of assertions you made are simply *not true*.
It should be made clear that the current OpenMP runtime CMake build
system has been in development for some time, including on-list
discussions in the LLVM community that go back weeks following all
the best practices we have. The only thing that changed is that C.
Bergstrom graciously provided the sign-off we needed to integrate
Jack's work late last week.What "discussions... that go back weeks" you are speaking about?!
Jack started his "On Improving the Build System revisited" thread on
May 30. This is four days ago, not weeks.And since when "all the best practices" include introducing a new
build system without getting project architect's consent? --
especially after explicitly asked to do so, a message that you
conveniently ignored.
To which message are you referring?
So it's a mischaracterisation to say this happened over the weekend.
Even if it did that would be on the long side compared to timescales
seen on llvm-commits.What timescales you are speaking about?!
For reference, we wait for *weeks* for our OpenMP in clang patches to
be reviewed! And we commit them *only* after explicit consent of one
of clang code owners -- even if we already got code review from
someone else.
To be fair, this is a bit different. First, as I've explained, you don't need approval from the code owner, you need approval from some established contributor who is knowledgeable in that part of the code, including design, implementation details and future direction (and, in effect, who is willing to take responsibility for your change). The problem you have with many of the OpenMP patches is that only the code owners are in such a position, but please don't over-generalize. There are many more qualified people for build system changes (although, FWIW, not as many as one might think). Please understand that, as far as we could tell, the future direction (CMake) was clear, and so Alp had all of the knowledge he needed to approve the commit.
In general it's a good idea to participate in on-list discussions and
give a heads up if you see people discussing features you have plans
for. Is there anything else in the pipeline?That's *exactly* what we did back in March.
http://lists.cs.uiuc.edu/pipermail/openmp-dev/2014-March/000055.html
I think the question is more general. Yes, the build system question was posed on the list, but the implementation roadmap was not. The roadmap for the library in general needs to be an open discussion.
-Hal