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