This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP50]Generalize handling of context matching/scoring.
ClosedPublic

Authored by ABataev on Nov 7 2019, 9:32 AM.

Details

Summary

Untie context matching/scoring from the attribute for declare variant
directive to simplify future uses in other context-dependent directives.

Event Timeline

ABataev created this revision.Nov 7 2019, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2019, 9:32 AM
Herald added a subscriber: guansong. · View Herald Transcript
jdoerfert added inline comments.Nov 7 2019, 10:28 AM
clang/include/clang/Basic/Attr.td
3310

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?

clang/include/clang/Basic/OpenMPKinds.def
226

Can we add this into llvm/include/llvm/IR/OpenMPKinds.def from the very beginning?

clang/include/clang/Basic/OpenMPKinds.h
43

I feel uneasy about using an ArrayRef for storage as it can easily lead to UB when the underlying values go out of scope.

52

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.

56

Why do you need a second template version here?

57

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.

ABataev marked 6 inline comments as done.Nov 7 2019, 10:40 AM
ABataev added inline comments.
clang/include/clang/Basic/Attr.td
3310

Nope, attributes td supports only limited number of possible constructs,mostly because of the serialization/deserialization problem.

clang/include/clang/Basic/OpenMPKinds.def
226

When we have it in trunk, sure.

clang/include/clang/Basic/OpenMPKinds.h
43

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).

52

No, mostly it will point to the data stored in the attribute, which exist all the time the function it is attached to exists.

56

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)

57

What scope are you ralking about?

jdoerfert added inline comments.Nov 7 2019, 11:32 AM
clang/include/clang/Basic/Attr.td
3310

That is what I was afraid of.

clang/include/clang/Basic/OpenMPKinds.def
226

Let's get it in then :)

clang/include/clang/Basic/OpenMPKinds.h
43

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.

52

See above.

56

That can be addressed by changing the first constructor. Why is it an xvalue there and why is the container not const?

57

"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.

ABataev marked 3 inline comments as done.Nov 7 2019, 11:42 AM
ABataev added inline comments.
clang/include/clang/Basic/OpenMPKinds.h
43

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.

56

In short, to avoid some extra memory allocation/deallocation. I can make the container const, ok.

57

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?

ABataev marked an inline comment as done.Nov 7 2019, 12:09 PM
ABataev added inline comments.
clang/include/clang/Basic/OpenMPKinds.h
56

Tried to make it const, it breaks a lot of code and almost impossible to fix

ABataev updated this revision to Diff 228290.Nov 7 2019, 12:25 PM

Rebase + address some comments.

jdoerfert added inline comments.Nov 7 2019, 1:58 PM
clang/include/clang/Basic/OpenMPKinds.h
43

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.

56
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.

57

Yes, please. That ties the score directly to the name.

ABataev marked 3 inline comments as done.Nov 7 2019, 2:11 PM
ABataev added inline comments.
clang/include/clang/Basic/OpenMPKinds.h
43

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

56

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.

57

Ok, will do.

ABataev updated this revision to Diff 228481.Nov 8 2019, 9:52 AM

Score is part of context data.

I think we are almost there.

Last remaining points:

  • Constructor (see below)
  • enum naming scheme (see below)
  • can we test this somehow?
clang/include/clang/Basic/OpenMPKinds.h
43

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.

56

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()) {}
clang/lib/Basic/OpenMPKinds.cpp
66

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.

ABataev marked 2 inline comments as done.Nov 8 2019, 1:38 PM

I think we are almost there.

Last remaining points:

  • Constructor (see below)
  • enum naming scheme (see below)
  • can we test this somehow?

We have the tests already for the declare variant. The fact that they are not modified shows the correctness of this patch. This patch is actually an NFC patch, just a redesign to make the solution more portable for future work with the OpenMP contexts in other constructs and unify analysis of the contexts selectors between all possible constructs.

clang/include/clang/Basic/OpenMPKinds.h
56

No. With this kind of constructor, we'll need to explicitly convert iterator_range or other containers to the VectorType and create new temp variable just to do simple copying. The existing constructor aallows to create VectorType from other containers, not only from VectorType

clang/lib/Basic/OpenMPKinds.cpp
66

Ok, will do.

ABataev updated this revision to Diff 228690.Nov 11 2019, 7:12 AM

Address comments

This revision is now accepted and ready to land.Nov 11 2019, 11:33 AM
This revision was automatically updated to reflect the committed changes.