<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 28, 2015 at 2:31 PM, Andrey Bokhanko <span dir="ltr"><<a href="mailto:andreybokhanko@gmail.com" target="_blank">andreybokhanko@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Chandler,<br>
<br>
Issue #1 -- not an issue at all. Personally, I *welcome* your fixes,<br>
and it's logical they are coming from you -- as apart of users you<br>
mentioned, there are zero documented users who *rely* on clang linking<br>
libgomp and ignoring all OMP pragmas.<br>
<br>
As for lack of code review, I trust your judgment and experience here<br>
(and still (!) ready to learn from you... -- remember?) and more than<br>
trust your driver fu.<br>
<br>
Issue #2 -- yes, I'm concerned. This seriously affects "the shortest<br>
path" user interface to OMP implementation and runtime library. I<br>
believe it warrants at least a discussion and some kind of consent in<br>
the community. Obviously, involving code owners of said OMP<br>
implementations would be reasonable.<br>
<span class=""><br>
> 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.<br>
<br>
</span>That's surprising. We discussed this with Alexey and the very next day<br>
he responded with a fix that (as we hoped) resolves your concerns and<br>
gives an easy way to configure clang the way you and your users like.<br>
You replied to this fix with:<br>
<br>
> Yea, i think Daniel's patch was a closer start. Anyways, thanks for taking<br>
> a stab at this.<br>
<br>
(from <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9875&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=S3-G1iz3zPO0V3Q2giAI7gLxRpbD6Z6EVkCXbZqEDgo&s=T5wAxyDOlMuVRBL2DvxJeC-HjhxgUaNcE7DCt6x91es&e=" target="_blank">http://reviews.llvm.org/D9875</a>)<br>
<br>
I interpreted it as "I'm not 100% happy but that's OK". Is my<br>
interpretation wrong?<br>
<br>
I *really* don't want us to become two boys shouting "it is libgomp!<br>
flip" and "no, it is libiomp! flip" to each other and flipping the<br>
switch ad infinitum. But I frankly fail to understand your logic...</blockquote><div><br></div><div>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):</div><div><br></div><div>* 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.</div><div>* Update the clang "getting started" documentation to suggest doing this if the user wants OpenMP support (change <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__clang.llvm.org_get-5Fstarted.html&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=S3-G1iz3zPO0V3Q2giAI7gLxRpbD6Z6EVkCXbZqEDgo&s=UWr_DP36WqFTyolrF5n6k8iR-G4LklN9apeIWM1YZZY&e=">http://clang.llvm.org/get_started.html</a> to say what to check out and where -- no steps other than an 'svn co' should be necessary).<br></div><div>* Change the default for CLANG_DEFAULT_OPENMP_RUNTIME to libomp (possibly conditioned on a "is libomp part of this build?" test).<br></div><div><br></div><div>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).</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><font color="#888888">
Andrey<br>
</font></span><div class=""><div class="h5"><br>
On Thu, May 28, 2015 at 11:56 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
> On Thu, May 28, 2015 at 5:48 AM Andrey Bokhanko <<a href="mailto:andreybokhanko@gmail.com">andreybokhanko@gmail.com</a>><br>
> wrote:<br>
>><br>
>> Chandler,<br>
>><br>
>> > +set(CLANG_DEFAULT_OPENMP_RUNTIME "libgomp" CACHE STRING<br>
>> > +  "Default OpenMP runtime used by -fopenmp.")<br>
>><br>
>> This effectively invalidates instructions recently published on LLVM<br>
>> blog (<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__blog.llvm.org_2015_05_openmp-2Dsupport-5F22.html&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=S3-G1iz3zPO0V3Q2giAI7gLxRpbD6Z6EVkCXbZqEDgo&s=zeM0Xwd5JyaHVUphQ6DSOnL3g6-uSNLft6NGfmmthqU&e=" target="_blank">http://blog.llvm.org/2015/05/openmp-support_22.html</a>) and picked<br>
>> up in many places.<br>
><br>
><br>
> I don't think there is anything we can do about this. =/ That's the risk of<br>
> publishing widely about stuff that has just barely landed in the tree.<br>
><br>
>><br>
>><br>
>> > To re-enable LLVM's OpenMP runtime (which I think should wait until the<br>
>> > normal getting started instructions are a reasonable way for falks to<br>
>> > check out, build, and install Clang with the runtime)<br>
>><br>
>> Why this readme: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__openmp.llvm.org_README.txt&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=S3-G1iz3zPO0V3Q2giAI7gLxRpbD6Z6EVkCXbZqEDgo&s=8W1bBfaP0VPekFlJFyzfqOUCWCWChbyXAi58IdNwHCk&e=" target="_blank">http://openmp.llvm.org/README.txt</a> (easily accessible<br>
>> from <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__openmp.llvm.org&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=S3-G1iz3zPO0V3Q2giAI7gLxRpbD6Z6EVkCXbZqEDgo&s=xhE1pc6l_maf6yMgh7rorX8phJEa38-36NIlI5a10dA&e=" target="_blank">openmp.llvm.org</a>) is not sufficient?<br>
><br>
><br>
> Why would someone trying to get LLVM or Clang read the OpenMP readme? Clang<br>
> *directly* supports the '-fopenmp' flag, so I wouldn't expect them to go<br>
> read other files.<br>
><br>
> Look at the instructions around compiler-rt -- we're very clear that parts<br>
> of Clang actually rely on these runtime libraries.<br>
><br>
> I'll even help update all of these documents once the cleanups and fixes to<br>
> the CMake and lit infrastructure land, and it's reasonably unintrusive to<br>
> grab the OpenMP runtime along with Clang and LLVM and build all of it.<br>
><br>
>><br>
>><br>
>> Also, have you reviewed your patch with OpenMP in Clang code owner<br>
>> (Alexey Bataev)?<br>
><br>
><br>
> No, and that is never a requirement really.<br>
><br>
> Code owners don't have *exclusive* rights to approve patches. That's not the<br>
> point of a code owner in LLVM. They are a person of last resort if a<br>
> contributor can't find someone else to review the patch.<br>
><br>
>> Or at all?...<br>
><br>
><br>
> I have worked *extensively* on the Clang driver and am very comfortable<br>
> committing these kinds of improvements and getting post-commit review. And<br>
> you'll see some nice post commit review on this thread.<br>
><br>
> I'll point out that my patch added significant missing testing, fixed<br>
> numerous problems, and had much more comprehensive documentation than the<br>
> original patch.<br>
><br>
><br>
> There are ultimately two separate issues here:<br>
><br>
> 1) The *mechanisms* for controlling openmp support were not correctly<br>
> implemented and needed to be restructured, fixed, and tested. This patch<br>
> does all of that, and 99.9% of it is concerned with these aspects.<br>
><br>
> 2) The default was flipped back to not relying on the iomp5 runtime.<br>
><br>
> I don't think #1 is controversial at all, but if you do, we can discuss that<br>
> more.<br>
><br>
> I think #2 may be a bit controversial, but I actually tried to reach out to<br>
> the person who flipped the default the first time -- Alexey -- in<br>
> post-commit review before doing anything. I did this *over a week ago*.<br>
> There was no response on that thread, and nothing was done to address the<br>
> concerns I raised.<br>
><br>
> The default flip actually regressed a very large number of my users.<br>
> Regressing users is *not* OK. Given the lack of response from the author, I<br>
> think I was entirely justified returning the default to its previous state.<br>
</div></div><div class=""><div class="h5">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>