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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked 3 inline comments as done.
ABataev added inline comments.


================
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 {
----------------
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.


================
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:
> > > 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.


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:57
+      : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
+};
+
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > It seems we always associate a scope with this class (even if the type of the scope varies), right? If so we can add it into this class to simplify the interfaces and tie them together in a nicer way.
> > What scope are you ralking about?
> "scope" was me thinking about the above issues while I was writing this comment about the "score":
> 
> It seems we always associate a score with this class (even if the type of the score varies), right? If so we can add it into this class to simplify the interfaces and tie them together in a nicer way.
Ah, I see. We store scores in the different representations in different places. In parsing/sema the score is represented as ExprResult, while in codegen it is represented as llvm::APSInt. It means that we need to introduce another one template parameter for the score. Are you ok with the third template param for the type of the score?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69952