[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

[Alexey Bataev via Phabricator via cfe-commits]

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