ABataev added a comment. In D71179#1775442 <https://reviews.llvm.org/D71179#1775442>, @jdoerfert wrote:
> > @jdoerfert , how does the ".ompvariant" work with external functions? I see > > the part of the spec which says, "The symbol name of a function definition > > that appears between a begin declare variant...", but, if we append this > > name to, for example, the names of functions present in the device math > > library, won't we have a problem with linking? > > We restricted it for now to function definitions so we don't need to define > the mangling as you cannot expect linking. (I did this to get it in TR8 while > I figured it will solve all our math.h problems already). > However, we need to avoid collisions with user code, e.g., through the use > of symbols in the name that are not allowed to be used by the user (I thought > "." is one of them). > > In D71179#1774639 <https://reviews.llvm.org/D71179#1774639>, @JonChesterfield > wrote: > > > Great to see the fragile math.h stuff disappear. > > > > I'm not sure about the CPU/GPU/other granularity. An openmp program with > > x86 as the host and target offload regions for amdgcn and for nvptx seems > > like a reasonable aspiration. Or for a couple of different generations from > > the same vendor. > > > > More ambitiously, one might want a GPU to be the host, and offload kernels > > for I/O to an aarch64 "target". > > > > We don't need to wire such combinations in up front, and I don't think > > they're excluded by this design. A future 'x86-64' variant would presumably > > be chosen over a 'cpu' variant when compiling for x86-64. > > > As I wrote in the inline comment somewhere, `kind(gpu)` is an artifact due to > missing fine-grained context selectors. If that wasn't the core of your > issue, please elaborate. > > In D71179#1775157 <https://reviews.llvm.org/D71179#1775157>, @ABataev wrote: > > > In D71179#1775066 <https://reviews.llvm.org/D71179#1775066>, @hfinkel wrote: > > > > > In D71179#1774678 <https://reviews.llvm.org/D71179#1774678>, @ABataev > > > wrote: > > > > > > > In D71179#1774487 <https://reviews.llvm.org/D71179#1774487>, @jdoerfert > > > > wrote: > > > > > > > > > In D71179#1774471 <https://reviews.llvm.org/D71179#1774471>, @ABataev > > > > > wrote: > > > > > > > > > > > They do this because they have several function definitions with > > > > > > the same name. In our case, we have several different functions > > > > > > with different names and for us no need to worry about overloading > > > > > > resolution, the compiler will do everything for us. > > > > > > > > > > > > > > > I think we talk past each other again. This is the implementation of > > > > > `omp begin/end declare variant` as described in TR8. Bt definition, > > > > > the new variant mechanism will result in several different function > > > > > definitions with the same name. See the two tests for examples. > > > > > > > > > > > > I just don't get it. If begin/end is just a something like > > > > #ifdef...endif, why you just can't skip everything between begin/end if > > > > the context does not match? > > > > > > > > > The patch does this (see in ParseOpenMP.cpp where I asked about the > > > potential inf-loop). But when the definitions are not skipped, then we > > > have to worry about having multiple decls/defs of the same name and the > > > overload priorities. > > > > > > I would recommend to drop all this extra stuff from the patch and focus on > > the initial patch. We'll need something similar to multiversion in case of > > the construct context selectors, but at first we need to solve all the > > problems with the simple versions of the construct rather that try to solve > > all the problems in the world in one patch. It is almost impossible to > > review. > > > I agree with you to the point that this is not supposed to be reviewed. > That's why I wrote that in the commit message. I did this so we can make sure > the general path is clear and people (myself included) can see how/that it > works. > I also agree that construct context selectors are very close to > multi-versioned functions. That is why I said earlier we should move all > variant handling into this scheme. I don't think we should do this. Something similar to multiversioning is required only for a small subset. Everything else can be implemented in a more straightforward and simple way. Plus, I'm not sure that we'll need full reuse of the multiversioning. Seems to me, we can implement codegen in a different way. Multiversioning is supported only by x86 in clang/LLVM. I think we can try to implement a more portable and universal scheme. > My plan: > > - We play around with this prototype now, make sure there are no major > problems with it (so far it didn't seem so). > - We split it up (This doesn't necessarily need to be only done by me, as > that often slows down these processes). > - We review the parts with proper test coverage, etc. and get it in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
