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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I think we are almost there.

Last remaining points:

- Constructor (see below)
- enum naming scheme (see below)
- can we test this somehow?



================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:43
+/// Struct to store the context selectors info.
+template <typename T, typename VectorType = llvm::ArrayRef<T>>
+struct OpenMPCtxSelectorData {
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > I feel uneasy about using an ArrayRef for storage as it can easily lead to UB when the underlying values go out of scope.
> > > > > This structure is used in 2 ways: 1. Store the data befoere creating the attribute (which will store all the data). 2. Generalize data later atthe codegen phase without allocating new memory (the data is alredy stored in the attribute and don't need to allocate it several times).
> > > > That might all be true but it does not address my comments. I argue that this is easy to be misused not that it will not work right now. For me this is a potentially misused performance improvement, a combination we should not introduce.
> > > > 
> > > > You could, for example, remove the ArrayRef default parameter and make it explicit at the instantiation sites. Though, I'd actually prefer owning the data here, e.g., in a SmallVector.
> > > I can remove the default parameter, sure. But hen the attribute created already, we don't need to create a new container, we can use ArrayRefs which may point to the memory allocated for the attribute without danger of undefied behavior.
> > My point is that there is always "danger of UB" if you point to memory allocated somewhere else. At the end of the day, this is just a container but one that may or may not own the underlying memory. If it does, people can use it as they use any other container. If it does not, people may still sue it as they use any other container but changes to the lifetime of the underlying memory will potentially cause UB down the line.
> > 
> > There is little incentive to open up potential error sources only to optimize for performance of something that is very likely not even visible in a profile. I mean, we will not see the impact of am additional memcpy of `sizeof(unsigned) * #ContextSelectors` bytes, or anything similar.
> It is not about the performance rather than to reduce mem usage. We may have a lot of context selectors, very complex, associated woth the same function. And I don't think it is a bad idea to reuse the previously allocated memory rather than allocate a new one and increase the memory pressure for the compiler
I am not convinced we would see a significant difference in memory consumption either.
However, as I mentioned, not having ArrayRef as a default template argument is probably good enough.


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:56
+                                 OpenMPContextSelectorKind Ctx, const U &Names)
+      : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
+};
----------------
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()) {}
```



================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:66
+}
+
 OpenMPDirectiveKind clang::getOpenMPDirectiveKind(StringRef Str) {
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69952