ABataev marked 6 inline comments as done.
ABataev added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:3310
+ VariadicUnsignedArgument<"CtxSelectorSets">,
+ VariadicUnsignedArgument<"CtxSelectors">,
VariadicStringArgument<"ImplVendors">
----------------
jdoerfert wrote:
> Is it possible to have a `VariadicObject` type here to represent the
> hierarchy and the connection between scores, selector sets, and selectors in
> a more explicit way?
Nope, attributes td supports only limited number of possible constructs,mostly
because of the serialization/deserialization problem.
================
Comment at: clang/include/clang/Basic/OpenMPKinds.def:226
+// OpenMP context selectors.
+OPENMP_CONTEXT_SELECTOR(vendor)
----------------
jdoerfert wrote:
> Can we add this into `llvm/include/llvm/IR/OpenMPKinds.def` from the very
> beginning?
When we have it in trunk, sure.
================
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:
> 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).
================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:52
+ VectorType &&Names)
+ : CtxSet(CtxSet), Ctx(Ctx), Names(Names) {}
+ template <typename U>
----------------
jdoerfert wrote:
> This constructor, together with the default vector type (ArrayRef), will most
> certainly lead to problems when used. Given that Names is an xvalue, it is
> basically at the end of its lifetime. If the user did not change the ArrayRef
> to sth that owns the data, Names will contain stale references.
No, mostly it will point to the data stored in the attribute, which exist all
the time the function it is attached to exists.
================
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:
> 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)
================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:57
+ : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
+};
+
----------------
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?
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
