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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

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. We should split this into several patches (e.g., basic handling, skipping parsing for incompatible selectors, overload things). I think that @jdoerfert posted this so that people can see the high-level direction and provide feedback (including feedback on how to stage the functionality for review).

@jdoerfert , also, do we have tests that can go into the test suite / libomptarget regression tests demonstrating the collection of problems people have currently opened bugs on regarding math.h? I recall we still had problems with host code needing the long-double overloads, with constants from the system headers, etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71179/new/

https://reviews.llvm.org/D71179