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:
> > > 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
================
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:
> > 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.
================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:57
+ : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
+};
+
----------------
jdoerfert wrote:
> ABataev wrote:
> > 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?
> Yes, please. That ties the score directly to the name.
Ok, will do.
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
