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

[Andrey Bokhanko]

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...

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.