r238389 - [omp] Re-work Clang's handling of -fopenmp and undo r237769.

[Andrey Bokhanko]

+Johnny, as it is up to him to make first two steps. :-)

Richard, you plan makes perfect sense to me.

Andrey


On Fri, May 29, 2015 at 1:05 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Thu, May 28, 2015 at 2:31 PM, Andrey Bokhanko <andreybokhanko at gmail.com>
> wrote:
>>
>> Chandler,
>>
>> Issue #1 -- not an issue at all. Personally, I *welcome* your fixes,
>> and it's logical they are coming from you -- as apart of users you
>> mentioned, there are zero documented users who *rely* on clang linking
>> libgomp and ignoring all OMP pragmas.
>>
>> As for lack of code review, I trust your judgment and experience here
>> (and still (!) ready to learn from you... -- remember?) and more than
>> trust your driver fu.
>>
>> Issue #2 -- yes, I'm concerned. This seriously affects "the shortest
>> path" user interface to OMP implementation and runtime library. I
>> believe it warrants at least a discussion and some kind of consent in
>> the community. Obviously, involving code owners of said OMP
>> implementations would be reasonable.
>>
>> > The default flip actually regressed a very large number of my users.
>> > Regressing users is *not* OK. Given the lack of response from the author, I
>> > think I was entirely justified returning the default to its previous state.
>>
>> That's surprising. We discussed this with Alexey and the very next day
>> he responded with a fix that (as we hoped) resolves your concerns and
>> gives an easy way to configure clang the way you and your users like.
>> You replied to this fix with:
>>
>> > Yea, i think Daniel's patch was a closer start. Anyways, thanks for
>> > taking
>> > a stab at this.
>>
>> (from http://reviews.llvm.org/D9875)
>>
>> I interpreted it as "I'm not 100% happy but that's OK". Is my
>> interpretation wrong?
>>
>> I *really* don't want us to become two boys shouting "it is libgomp!
>> flip" and "no, it is libiomp! flip" to each other and flipping the
>> switch ad infinitum. But I frankly fail to understand your logic...
>
>
> I'm not sure it's worth changing the default to libiomp5 given that the
> rename to libomp is imminent; it'd be better to do the rename first and then
> switch the default. Here's what I suggest as a path forwards (and sorry if
> I'm not up to speed on everything, I've just come back from a week on
> vacation):
>
> * Reach the point where the openmp runtime library can be checked out into a
> normal llvm / clang build tree (into projects/openmp, perhaps) and it
> integrates properly into the build and builds successfully on various
> systems.
> * Update the clang "getting started" documentation to suggest doing this if
> the user wants OpenMP support (change http://clang.llvm.org/get_started.html
> to say what to check out and where -- no steps other than an 'svn co' should
> be necessary).
> * Change the default for CLANG_DEFAULT_OPENMP_RUNTIME to libomp (possibly
> conditioned on a "is libomp part of this build?" test).
>
> That first bullet really is important -- we want the -fopenmp to work "out
> of the box" for people building from SVN, so it should be trivially easy to
> build a Clang with full OpenMP support, including building the runtime
> library as part of the normal build (as we currently do for the C++ runtime,
> the sanitizer runtime, and so on).
>
>> Andrey
>>
>> On Thu, May 28, 2015 at 11:56 PM, Chandler Carruth <chandlerc at gmail.com>
>> wrote:
>> > On Thu, May 28, 2015 at 5:48 AM Andrey Bokhanko
>> > <andreybokhanko at gmail.com>
>> > wrote:
>> >>
>> >> Chandler,
>> >>
>> >> > +set(CLANG_DEFAULT_OPENMP_RUNTIME "libgomp" CACHE STRING
>> >> > +  "Default OpenMP runtime used by -fopenmp.")
>> >>
>> >> This effectively invalidates instructions recently published on LLVM
>> >> blog (http://blog.llvm.org/2015/05/openmp-support_22.html) and picked
>> >> up in many places.
>> >
>> >
>> > I don't think there is anything we can do about this. =/ That's the risk
>> > of
>> > publishing widely about stuff that has just barely landed in the tree.
>> >
>> >>
>> >>
>> >> > To re-enable LLVM's OpenMP runtime (which I think should wait until
>> >> > the
>> >> > normal getting started instructions are a reasonable way for falks to
>> >> > check out, build, and install Clang with the runtime)
>> >>
>> >> Why this readme: http://openmp.llvm.org/README.txt (easily accessible
>> >> from openmp.llvm.org) is not sufficient?
>> >
>> >
>> > Why would someone trying to get LLVM or Clang read the OpenMP readme?
>> > Clang
>> > *directly* supports the '-fopenmp' flag, so I wouldn't expect them to go
>> > read other files.
>> >
>> > Look at the instructions around compiler-rt -- we're very clear that
>> > parts
>> > of Clang actually rely on these runtime libraries.
>> >
>> > I'll even help update all of these documents once the cleanups and fixes
>> > to
>> > the CMake and lit infrastructure land, and it's reasonably unintrusive
>> > to
>> > grab the OpenMP runtime along with Clang and LLVM and build all of it.
>> >
>> >>
>> >>
>> >> Also, have you reviewed your patch with OpenMP in Clang code owner
>> >> (Alexey Bataev)?
>> >
>> >
>> > No, and that is never a requirement really.
>> >
>> > Code owners don't have *exclusive* rights to approve patches. That's not
>> > the
>> > point of a code owner in LLVM. They are a person of last resort if a
>> > contributor can't find someone else to review the patch.
>> >
>> >> Or at all?...
>> >
>> >
>> > I have worked *extensively* on the Clang driver and am very comfortable
>> > committing these kinds of improvements and getting post-commit review.
>> > And
>> > you'll see some nice post commit review on this thread.
>> >
>> > I'll point out that my patch added significant missing testing, fixed
>> > numerous problems, and had much more comprehensive documentation than
>> > the
>> > original patch.
>> >
>> >
>> > There are ultimately two separate issues here:
>> >
>> > 1) The *mechanisms* for controlling openmp support were not correctly
>> > implemented and needed to be restructured, fixed, and tested. This patch
>> > does all of that, and 99.9% of it is concerned with these aspects.
>> >
>> > 2) The default was flipped back to not relying on the iomp5 runtime.
>> >
>> > I don't think #1 is controversial at all, but if you do, we can discuss
>> > that
>> > more.
>> >
>> > I think #2 may be a bit controversial, but I actually tried to reach out
>> > to
>> > the person who flipped the default the first time -- Alexey -- in
>> > post-commit review before doing anything. I did this *over a week ago*.
>> > There was no response on that thread, and nothing was done to address
>> > the
>> > concerns I raised.
>> >
>> > The default flip actually regressed a very large number of my users.
>> > Regressing users is *not* OK. Given the lack of response from the
>> > author, I
>> > think I was entirely justified returning the default to its previous
>> > state.
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>