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

[Richard Smith]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150528/f3929e2d/attachment.html>