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

Alexey Bataev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 9 06:13:32 PST 2019


ABataev added a comment.

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.



================
Comment at: clang/lib/AST/StmtOpenMP.cpp:2243
 }
+
+// TODO: We have various representations for the same data, it might help to
----------------
jdoerfert wrote:
> This code was basically only moved, not written for this patch. It needs to life somewhere accessible from Parser to CodeGen, see the TODOs below.
I don't think this is the right place for this code. Will try to move it to Basic directory in my patch.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1489
+        ++Nesting;
+    } while (Nesting);
+
----------------
hfinkel wrote:
> Will this just inf-loop if the file ends?
It will.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179





More information about the cfe-commits mailing list