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

[Chandler Carruth]

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