Alp,
I understand your emotions, but let us stick to the facts, not invent them.
There is nothing hostile or arrogant in my mail towards you – you still have my respect as an active member of llvm (and now libiomp :-)) community.
However, stating that you
Sounds great - I think the goals here extend just slightly past what is needed for clang and I'm eager to review the work. When you say Fortran - do you mean building the Fortran .mod (modules). I assume that without a Fortran compiler this can be disabled.. (please do make sure it's optional component)
If you really want to get fancy - we should figure out a couple things
1) Is the library intended to be built with the system compiler or the "just built" clang - assuming this integrates into the higher level build system like compiler-rt
2) Will it support cross compiling. (x86 host and PPC64 target)
Hal,
From: "Andrey Bokhanko" <andreybokhanko@gmail.com>
To: "Jack Howarth" <howarth.mailing.lists@gmail.com>
Cc: openmp-dev@dcs-maillist2.engr.illinois.edu, "David Chisnall" <dc552@cam.ac.uk>
Sent: Monday, June 2, 2014 11:59:57 AM
Subject: Re: [Openmp-dev] [PATCH] [Revisedx2] Initial cmake supportJack,
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.
Just to be clear (because I feel this is important), it is not a matter of authority, it is a matter of expertise. There are not many regular contributors with the expertise and background that would allow them to feel comfortable approving the patches. And, yes, this will improve as time goes on (and, if nothing else, based on current trends, the list of relevant people will increasingly come to include Alexey, etc.).
-Hal
Can you give a specific example? I want this to be exactly what you all need and want. So I want to be sure what you are talking about.
Johnny
I can't speak authoritatively for David, but I think he just meant he wanted to kick the perl dependency out. I hope perl is not involved at all in the new thing you are going to be proposing soon. (Same goes for pretty much any other script except maybe windows compatible posix sh)
From: "Andrey Bokhanko" <andreybokhanko@gmail.com>
To: "Hal Finkel" <hfinkel@anl.gov>
Cc: openmp-dev@dcs-maillist2.engr.illinois.edu, "David Chisnall" <dc552@cam.ac.uk>, "Alp Toker" <alp@nuanti.com>
Sent: Monday, June 2, 2014 12:16:56 PM
Subject: Re: [Openmp-dev] [PATCH] [Revisedx2] Initial cmake supportHal,
To which message are you referring?
This one:
http://lists.cs.uiuc.edu/pipermail/openmp-dev/2014-June/000146.htmlTo 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.Agree -- no two things are exactly the same.
Said this, do you think reviewing and committing a new build system
on a weekend, with no chance to have a review with project architect
is a good and collaborative approach?
There are two issues here. First, a specific request was made to await Jim's review (and, so, we should have waited). Had that not been the case, it is questionable. Alp is familiar with LLVM's build systems, and given that we had no knowledge of other work being done, I'm not going to fault him particularly (although, as I stated, we should have awaited Jim's feedback as was requested). Now that we know that others are actively working on the build system, we will naturally await their opinions for non-trivial changes.
-Hal
When I tried to compile on FreeBSD, I spent an hour fighting the build system before giving up. I was very glad that Alp jumped through the required hoops, but was even more glad that the person doing the hoop-jumping was not me. I'd like to see that the next person who tries to port the runtime to a new OS (Open/NetBSD, Haiku, whatever) doesn't have the same experience as me. Once you've got it to build, it's easy to see what the OS-specific parts are and tweak them, but the maze of twisty recursive perl-shell-make invocations all alike is a huge pain.
To see how hard it is with your new system, try adding support for building on NetBSD (it doesn't matter whether the code actually compiles there) and see how many *different* places you need to touch. This was alp's commit to bring up the initial FreeBSD port:
http://llvm.org/viewvc/llvm-project?view=revision&revision=202478
The files he needed to edit *just in the build system* were:
tools/common.inc
tools/freebsd.inc (and this one is a bit of an ugly hack - done correctly it would be factored out into a unix.inc that both linux.inc and freebsd.inc included)
lib/Platform.pm
lib/Uname.pm
src/makefile.mk
Having to modify the build system for a relatively small library in five different places is a significant barrier.
David
You are correct that is the .mod modules that are being built and they are OFF by default so a Fortran compiler is not required by default.
So far, testing has been done with system compilers (including production clang) only and not the "just built" clang compiler. I honestly don't know what is involved in getting the "just built" compiler to build it.
Cross compiling shouldn't be a problem as you basically call cmake as a configure step where you can specify the compiler to use and the architecture to build for, also, the "micro-tests" (test-touch, test-deps, etc.) which are done after building the library can be turned off easily so that you don't have to immediately use the library for a successful build like you do for build.pl.
I will consider turning the micro-tests off by default, although the touch-test can be useful for quickly finding segfaults when trying new code.
Johnny
Jonathan,
Thanks for the update. We had no idea that this work was in progress. It is unfortunate that an openmp repository at Intel (like that for clang-omp) doesn’t exist to monitor such work before it is submitted to llvm.org. In any case, the main changes that I implemented compared to the legacy cmake files on the openmprtl site are listed in…
Perl is involved in the recently added CMakeLists.txt... It is used to create header files (omp.h, *.inc, etc.) Until that dependency is removed it will always be needed.
Johnny
In our local cmake files it's not needed and the dependency should be removed (imho). I'll take a look there and see if there's something else we should contribute. Thanks for pointing this out
I’m truly sorry for the frustration. Unfortunately, I can’t release any of the code until it has gone through the entire review process here at Intel.
Johnny
Alp,
I understand your emotions, but let us stick to the facts, not invent them.
There is nothing hostile or arrogant in my mail towards you -- you still have my respect as an active member of llvm (and now libiomp :-)) community.
However, stating that you
technically the *top committer* on the LLVM openmp module given
that the "code drop" commit was monolithic),is simply bending the truth
Both statements I made there are truthful, and they were meant as mildly embarrassing commentary rather than a boast.
A good way to change the situation is to participate in on-list code review like everyone else and make a few commits. I note that despite all the discussion, there is still no alternative implementation posted to the public list.
and doesn't give you enough authority to approve significant changes to the project. Sorry.
Pretty sure it does, and others too.
http://llvm.org/docs/DeveloperPolicy.html
And besides, the CMake change doesn't introduce any change in functionality. As such, it's hardly a significant change in the sense a codegen or logic change would be.
These are text files that can be amended and improved with basic review, so let's go ahead and do that when you're ready to submit patches?
Finally,
$ git log --author=Andrey
commit c88ab54e0d3d89c97175d21d6af3466df5445eaa
Author: Andrey Churbanov <Andrey.Churbanov@intel.com
<mailto:Andrey.Churbanov@intel.com>>
Date: Thu Oct 3 07:27:25 2013 +0000typo fixed as a test commit
This is another Andrey. He works at Intel as well, but no relation to me.
Well, that drives the point home.
We've been maintaining the OpenMP runtime codebase here as part of the LLVM project and I look forward to helping review your changes, as I hope you'll come to review those of others with time.
I'm glad that there's an abundance of energy and opinions in a module that's been characterised by silence since October 2013 and we should work towards our shared goals
Alp.
From: "Jonathan L Peyton" <jonathan.l.peyton@intel.com>
To: "Jack Howarth" <howarth.mailing.lists@gmail.com>
Cc: openmp-dev@dcs-maillist2.engr.illinois.edu, "David Chisnall" <dc552@cam.ac.uk>
Sent: Monday, June 2, 2014 1:08:23 PM
Subject: Re: [Openmp-dev] [PATCH] [Revisedx2] Initial cmake supportI’m truly sorry for the frustration. Unfortunately, I can’t release
any of the code until it has gone through the entire review process
here at Intel.
Can you explain what is going on here? What kind of review (code, legal, both, something else) and by whom (I'm not asking for specific names)? Does this review prerequisite also apply to bug fixes? How long do you expect it to take?
Thanks,
Hal
Can you give a specific example? I want this to be exactly what you all need and want. So I want to be sure what you are talking about.
When I tried to compile on FreeBSD, I spent an hour fighting the build system before giving up. I was very glad that Alp jumped through the required hoops, but was even more glad that the person doing the hoop-jumping was not me. I'd like to see that the next person who tries to port the runtime to a new OS (Open/NetBSD, Haiku, whatever) doesn't have the same experience as me. Once you've got it to build, it's easy to see what the OS-specific parts are and tweak them, but the maze of twisty recursive perl-shell-make invocations all alike is a huge pain.
To see how hard it is with your new system, try adding support for building on NetBSD (it doesn't matter whether the code actually compiles there) and see how many *different* places you need to touch. This was alp's commit to bring up the initial FreeBSD port:
http://llvm.org/viewvc/llvm-project?view=revision&revision=202478
Right. To make the codebase itself more portable, a natural step was to move from KMP_OS ifdefs towards predication on features like KMP_AFFINITY_SUPPORTED:
I like David's idea of applying similar simplifications to the build system, which will improve locality and simplify porting. A bonus is that such a feature-based approach is self-documenting.
The concept of a predefined OS doesn't fit well for a low-level library like the OpenMP runtime, and even there it can vary based on what level of userspace you're at (e.g. the standard build vs. Many Integrated Core Architecture where FreeBSD is supported in two different senses).
This isn't a fundamental criticism, and the goals can be worked towards incrementally.
Alp.