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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
