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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits