<div dir="ltr"><div class="gmail_quote">On Thu, May 28, 2015 at 5:48 AM Andrey Bokhanko <<a href="mailto:andreybokhanko@gmail.com">andreybokhanko@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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=GY3yGKW5kaPQjShN8hvQgtGNs_uB-yolg9Onqf6LpxU&s=UVp_PNY1-jaiv7yJXBfx_mP6wK2Yz7w6Ajv3VlS241o&e=" target="_blank">http://blog.llvm.org/2015/05/openmp-support_22.html</a>) and picked<br>
up in many places.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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=GY3yGKW5kaPQjShN8hvQgtGNs_uB-yolg9Onqf6LpxU&s=i0aaX7GakCU9Nsm7FVnLE9O8Nlgj7OXvC9wcioRLLbM&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=GY3yGKW5kaPQjShN8hvQgtGNs_uB-yolg9Onqf6LpxU&s=q8QD5w0moN0JCQju9-qhlg9YonfWa__rjVPPT-Dil8o&e=" target="_blank">openmp.llvm.org</a>) is not sufficient?<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Look at the instructions around compiler-rt -- we're very clear that parts of Clang actually rely on these runtime libraries.</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, have you reviewed your patch with OpenMP in Clang code owner<br>
(Alexey Bataev)?</blockquote><div><br></div><div>No, and that is never a requirement really.</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Or at all?...<br></blockquote><div><br></div><div>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.</div><div><br></div><div>I'll point out that my patch added significant missing testing, fixed numerous problems, and had much more comprehensive documentation than the original patch.</div><div><br></div><div><br></div><div>There are ultimately two separate issues here:</div><div><br></div><div>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.</div><div><br></div><div>2) The default was flipped back to not relying on the iomp5 runtime.</div><div><br></div><div>I don't think #1 is controversial at all, but if you do, we can discuss that more.</div><div><br></div><div>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.</div><div><br></div><div>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.</div></div></div>