jdoerfert marked 5 inline comments as done. jdoerfert added a comment. > @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. 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
