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 <chandl...@gmail.com> wrote: > On Thu, May 28, 2015 at 5:48 AM Andrey Bokhanko <andreybokha...@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. _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
