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

[Andrey Bokhanko]

BTW, Alexey is on vacation and will be back on next week.

In the mean time, you have to deal with me... Why it is always me who
has to argue with Chandler? :-)

On Fri, May 29, 2015 at 12:31 AM, 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...
>
> 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.