[PATCH] D69952: [OPENMP50]Generalize handling of context matching/scoring.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked 2 inline comments as done.
ABataev added a comment.

In D69952#1739366 <https://reviews.llvm.org/D69952#1739366>, @jdoerfert wrote:

> I think we are almost there.
>
> Last remaining points:
>
> - Constructor (see below)
> - enum naming scheme (see below)
> - can we test this somehow?


We have the tests already for the declare variant. The fact that they are not modified shows the correctness of this patch. This patch is actually an NFC patch, just a redesign to make the solution more portable for future work with the OpenMP contexts in other constructs and unify analysis of the contexts selectors between all possible constructs.



================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:56
+                                 OpenMPContextSelectorKind Ctx, const U &Names)
+      : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
+};
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > Why do you need a second template version here?
> > > > > > > To construct the object when the container Names cannot be created dieectly using the first constructor (SmallVector and UniqueVector are not quite compatible in this sence, for example)
> > > > > > That can be addressed by changing the first constructor. Why is it an xvalue there and why is the container not const?
> > > > > In short, to avoid some extra memory allocation/deallocation. I can make the container const, ok.
> > > > Tried to make it `const`, it breaks a lot of code and almost impossible to fix
> > > ```
> > >   explicit OpenMPCtxSelectorData(OpenMPContextSelectorSetKind CtxSet,
> > >                                  OpenMPContextSelectorKind Ctx,
> > >                                  const VectorType &Names)
> > >       : CtxSet(CtxSet), Ctx(Ctx), Names(Names) {}
> > > ```
> > > 
> > > Is what I meant. It should also remove the need for the second constructor.
> > We may have a different comtainer type there, like `UniqurVector`, or itertor_range, for example, or something else, not completely compatible with the `VectorType` itself.
> Still, doesn't the below constructor obviate the need for the templated one?
> 
> ```
> OpenMPCtxSelectorData(OpenMPContextSelectorSetKind CtxSet,
>                                OpenMPContextSelectorKind Ctx,
>                                const VectorType &Names)
>     : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
> ```
> 
No. With this kind of constructor, we'll need to explicitly convert iterator_range or other containers to the `VectorType` and create new temp variable just to do simple copying. The existing constructor aallows to create `VectorType` from other containers, not only from `VectorType`


================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:66
+}
+
 OpenMPDirectiveKind clang::getOpenMPDirectiveKind(StringRef Str) {
----------------
jdoerfert wrote:
> I would have preferred to call the enums:
> ```
>   OMP_CTX_SET_{implementation,...}
>   OMP_CTX_{vendor,...}
> ```
> 
> I mean important things have `OMPX` names, like `OMPC_` for clauses and `OMPD_` for directives,
> but less important ones would all be `OMP_SOME_SHORT_DESIGNATOR_value`.
Ok, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69952