Page MenuHomePhabricator

[OpenMP][Part 2] Use reusable OpenMP context/traits handling
ClosedPublic

Authored by jdoerfert on Dec 23 2019, 1:03 AM.

Details

Summary

This patch implements an almost complete handling of OpenMP
contexts/traits such that we can reuse most of the logic in Flang
through the OMPContext.{h,cpp} in llvm/Frontend/OpenMP.

All but construct SIMD specifiers, e.g., inbranch, and the device ISA
selector are define in llvm/lib/Frontend/OpenMP/OMPKinds.def. From
these definitions we generate the enum classes TraitSet,
TraitSelector, and TraitProperty as well as conversion and helper
functions in llvm/lib/Frontend/OpenMP/OMPContext.{h,cpp}.

The above enum classes are used in the parser, sema, and the AST
attribute. The latter is not a collection of multiple primitive variant
arguments that contain encodings via numbers and strings but instead a
tree that mirrors the match clause (see struct OpenMPTraitInfo).

The changes to the parser make it more forgiving when wrong syntax is
read and they also resulted in more specialized diagnostics. The tests
are updated and the core issues are detected as before. Here and
elsewhere this patch tries to be generic, thus we do not distinguish
what selector set, selector, or property is parsed except if they do
behave exceptionally, as for example user={condition(EXPR)} does.

The sema logic changed in two ways: First, the OMPDeclareVariantAttr
representation changed, as mentioned above, and the sema was adjusted to
work with the new OpenMPTraitInfo. Second, the matching and scoring
logic moved into OMPContext.{h,cpp}. It is implemented on a flat
representation of the match clause that is not tied to clang.
OpenMPTraitInfo provides a method to generate this flat structure (see
struct VariantMatchInfo) by computing integer score values and boolean
user conditions from the clang::Expr we keep for them.

The OpenMP context is now an explicit object (see struct OMPContext).
This is in anticipation of construct traits that need to be tracked. The
OpenMP context, as well as the VariantMatchInfo, are basically made up
of a set of active or respectively required traits, e.g., 'host', and an
ordered container of constructs which allows duplication. Matching and
scoring is kept as generic as possible to allow easy extension in the
future.


Test changes:

The messages checked in `OpenMP/declare_variant_messages.{c,cpp}` have
been auto generated to match the new warnings and notes of the parser.
The "subset" checks were reversed causing the wrong version to be
picked. The tests have been adjusted to correct this.
We do not print scores if the user did not provide one.
We print spaces to make lists in the `match` clause more legible.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jdoerfert marked an inline comment as done.Feb 8 2020, 8:05 PM
jdoerfert added inline comments.
clang/include/clang/Serialization/ASTRecordReader.h
271

Moving the specialization to the source file seems to fix the issue.

Like this?

@kiranchandramohan Do you have more input on this one?

Looks OK to me.
Couple of comments inline.

clang/include/clang/Serialization/ASTRecordReader.h
271

You can remove the following declaration from the header file and just define it in the cpp file.
/// Specialization for OMPTraitInfo*.

template <> OMPTraitInfo *readUserType();
llvm/lib/Frontend/OpenMP/OMPContext.cpp
417

If there is no match? Or is it always guaranteed to have a match?

clang/lib/Serialization/ASTWriter.cpp
6581

Had to also move this up in the file to avoid an instantiation after use error.

jdoerfert marked 4 inline comments as done.Feb 11 2020, 5:22 PM

@kiranchandramohan @JonChesterfield I will address the comments just made, if you think this is otherwise fine it would be good if you accept the patch (it's been around for weeks so it's not really people didn't have a chance to look).

clang/include/clang/Serialization/ASTRecordReader.h
271

Right, will do that, thx!

clang/lib/Serialization/ASTWriter.cpp
6581

I didn't get that but I have it in the header, I'll figure it out.

llvm/lib/Frontend/OpenMP/OMPContext.cpp
417

We always have selector sets (the outermost category).

LGTM. You can wait for a day in case other reviewers have comments.

clang/lib/Serialization/ASTWriter.cpp
6581

You should remove the declaration of this specialization from the header file first.
Then move this definition of the specialization up in the file.

This revision is now accepted and ready to land.Feb 12 2020, 3:09 AM
This revision now requires review to proceed.Feb 12 2020, 3:32 AM
JonChesterfield added a comment.EditedFeb 12 2020, 3:49 AM

Procedural note - adding someone as a blocking reviewer to someone else's patch isn't great. What if the new reviewer never gets around to looking at the patch?

I've adjusted that to non-blocking, but feel free to leave a comment if I've missed something.

JonChesterfield removed 1 blocking reviewer(s): aaron.ballman.Feb 12 2020, 3:51 AM
This revision is now accepted and ready to land.Feb 12 2020, 3:51 AM

Procedural note - adding someone as a blocking reviewer to someone else's patch isn't great. What if the new reviewer never gets around to looking at the patch?

I've adjusted that to non-blocking, but feel free to leave a comment if I've missed something.

I don't know that we have any formalized procedure here, but setting a code owner as the blocking reviewer in phab makes it crystal clear that the existing LG(s) are insufficient for some reason and someone needs to do a final sign-off. It solves the problem of getting a LG from someone who may not be familiar with the code base and then making a problematic commit by accident.

clang/include/clang/AST/OpenMPClause.h
6662

datastructure -> data structure

6684

infor -> info

6692

humand -> human

clang/include/clang/Basic/Attr.td
185

The name seems a bit strange given that it's not a generic pointer, you specify a type with it. It's unclear whether you are supposed to specify the pointer type or the base type, which would help a bit. However, it's unclear to me why we'd want this generic mechanism in the first place. These classes are intended to help generate code to do checking automatically, and this design is somewhat at odds with that goal compared to exposing a custom argument type like we did for VersionArgument. Also, this type is marked optional rather than leaving that up to the caller, and the default behavior of a generic pointer argument always being default seems suspect.

I'm not saying "get rid of this", just wondering if you had considered the more verbose approach and had strong rationale as to why this was a better way.

3366

Just double-checking -- this is not a breaking change to existing code, correct?

It's not yet clear to me how a "generic pointer argument" relates to an OMPTraitInfo object that the end programmer will have no understanding of because it's a compiler implementation detail.

I used to understand what this attribute accepted as arguments and I no longer have the foggiest idea, which seems somewhat problematic.

clang/include/clang/Basic/DiagnosticParseKinds.td
1261

80-col limit

Also, I think the comma should be a semicolon so it reads ...describing a context set; selector skipped

1266

Turn the comma into a semicolon

1269

80-col limit; please run the patch through clang-format (I'll stop commenting on these).

1269

parenthesis -> parentheses

1276

Turn the comma into a semicolon

1279

Turn the comma into a semicolon

1282

Turn the comma into a semicolon (same comment in the other diagnostics, I'll stop commenting on these).

clang/lib/AST/OpenMPClause.cpp
1728–1729

Is there any reason to be worried about the O(N^2) here?

1741

I didn't see anything that validated that this is known to be a constant expression. Did I miss something?

Also, is there anything checking that the expression is not value dependent?

1742–1745

I think VMI.addTrait(CondVal.isNullValue() ? ... : ...); would be slightly more clear.

1752

Same question here.

1755

Hopefully the O(N^3) is also not a concern?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
11087

Is this change planned for the patch? Or is this a FIXME suggesting a better approach we hope to take someday?

clang/lib/Parse/ParseOpenMP.cpp
866

It is unfortunate how much this is duplicated. Perhaps make a free function to do this that gets called instead of copy pasting?

886

FIXME?

1056

Please don't use auto when the type is not obvious from the initialization.

1060

parenthesis -> parentheses

1368–1377

What is responsible for freeing this memory?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
430

What is responsible for freeing this memory?

clang/lib/Serialization/ASTReader.cpp
12617

What is responsible for freeing this memory?

clang/utils/TableGen/ClangAttrEmitter.cpp
110

The idea here being that if someone adds a GenericPointerArg, they'll have to add a specialization of readUserType<> for that named pointer type or else get template instantiation errors because there's no definition for the primary template?

369

Can we leave the unreachable in there and add actual checking for this case? The unreachable is intended to remind people "do this work when you're mucking about in here" and silently doing something that may or may not be wrong makes this harder to spot.

jdoerfert marked 14 inline comments as done.Feb 12 2020, 1:31 PM
jdoerfert added inline comments.
clang/include/clang/Basic/Attr.td
185

The name seems a bit strange given that it's not a generic pointer, you specify a type with it.

The "template" is generic as it accepts any pointer type. All the code that work with GenericPointerArgument don't know what pointer it is, just that it is called "Type". Don't you think that is generic?

Also, this type is marked optional rather than leaving that up to the caller, and the default behavior of a generic pointer argument always being default seems suspect.

That's an oversight. I mark it non-optional. I don't get the "always being default" part.

However, it's unclear to me why we'd want this generic mechanism in the first place.
[...]
I'm not saying "get rid of this", just wondering if you had considered the more verbose approach and had strong rationale as to why this was a better way.

The "problem" is that we want to keep "complex" information around. The verbose approach is what we are doing right now for a subset of the information. For this subset we already track various variadic expr, variadic unsigned, and variadic string arguments and stitch them together later with complex and careful iteration over all containers at the same time. It's now 5 variadic XXX arguments and it would be >= 7 to support what this approach does.

This approach subsumes this as we can retain the original structure/nesting in the custom OMPTraitInfo object that is part of the OMPDeclareVariant instead. [FWIW, in the review for the verbose code we have now, in case you haven't seen that, I asked for a less verbose method to store the information because the iteration over the stuff, once flattend, is so brittle.]

That said, we can make a specialized argument here instead, e.g., OMPTraitInforArgument, which contains a OMPTraitInfo pointer. I figured this might not be the last time someone wants to keep a complex structure inside an attribute argument and creating new arguments all the time seems a lot of waste (due to all the boilerplate). If you think we should go that route, I will happily try it out.

(As noted below somewhere, this method avoids a lot of boilerplate by requiring a specialization of one AST reader/writer method.)

3366

Just double-checking -- this is not a breaking change to existing code, correct?

This actually fixes various errors in the existing code and adds a lot of missing functionality, no regressions are known to me.

It's not yet clear to me how a "generic pointer argument" relates to an OMPTraitInfo object that the end programmer will have no understanding of because it's a compiler implementation detail.

This is not (supposed to be) user facing. This is generated from the ... in omp declare variant match(...).

I used to understand what this attribute accepted as arguments and I no longer have the foggiest idea, which seems somewhat problematic.

I don't understand? The attribute accepted a custom (=internal) encoding of an OpenMP context selector as a sequence of expressions, unsigneds, and strings. I do honestly not understand, without significant effort, how the nesting is rebuild from 5 such lists, e.g., how the iterators in the printPrettyPragma on the left (line 3359) work together. I argue OMPTraitInfo::print is much simpler because the nesting is retained.

clang/include/clang/Basic/DiagnosticParseKinds.td
1269

maybe we should tell git-clang-format to run on .td files (in the llvm subfolder)

clang/lib/AST/OpenMPClause.cpp
1728–1729

I don't think so. (1) There are only four sets, and a limited number of selectors. And, (2) all this does is go over user typed things which is not quadratic in the input size anyway. I means Sets and Set.Selectors are filled with things the user explicitly typed in a match clause.

1741

I didn't see anything that validated that this is known to be a constant expression. Did I miss something?

I'll look into that and make sure we check in the parser.

Also, is there anything checking that the expression is not value dependent?

That should be fine. If it is attached to a template it is instantiated with the template. Or am I missing something?

1752

Same question here.

I'll look into that and make sure we check in the parser.

1755

Same answer as for the N^2, this is only iterating over a user defined structure, not all possible values in an N^3 square.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
11087

It is something we need to do *soon* not someday, though it will only make sense as the OMPIRBuilder is gaining the ability to fully generate constructs.

clang/lib/Parse/ParseOpenMP.cpp
866

Will do.

1056

I want to avoid casting ParenCount and keep it in sync. I can copy whatever ParenCount is though.

1368–1377

Will check and fix if needed.

clang/utils/TableGen/ClangAttrEmitter.cpp
110

Correct.

aaron.ballman added inline comments.Feb 13 2020, 9:06 AM
clang/include/clang/Basic/Attr.td
185

That's an oversight. I mark it non-optional. I don't get the "always being default" part.

Making it non-optional solves my concern. I was just worried that the default was surprising.

That said, we can make a specialized argument here instead, e.g., OMPTraitInforArgument, which contains a OMPTraitInfo pointer. I figured this might not be the last time someone wants to keep a complex structure inside an attribute argument and creating new arguments all the time seems a lot of waste (due to all the boilerplate). If you think we should go that route, I will happily try it out.

As we spoke about on IRC, I would appreciate going with this approach (and I think it can even work nicely with the template specialization work done for the AST reader and writer, most likely). However, if that turns out to be a lot of effort for you, I can probably be okay with the current approach. I just dislike the fact that it complicates understanding what arguments the attribute actually takes (I no longer understand by looking at Attr.td or in the worst case, the tablegen emitter). Having the concrete type in tablegen/emitter is more expressive and allows us to generate more stuff in the future.

3366

I used to understand what this attribute accepted as arguments and I no longer have the foggiest idea, which seems somewhat problematic.

I don't understand?

My confusion comes from me thinking about arguments in the .td file as being descriptive of how a user will use the attribute (same for subjects and many other properties we set up on the attributes). So things like arguments are intended to be descriptive and self-documenting. In this case, it's no longer self-documenting or descriptive, or even user facing.

clang/include/clang/Basic/DiagnosticParseKinds.td
1269

Not a bad idea!

clang/lib/AST/OpenMPClause.cpp
1728–1729

Fantastic! That's what I was hoping to hear.

1741

That should be fine. If it is attached to a template it is instantiated with the template. Or am I missing something?

EvaluateKnownConstInt() asserts that the expression is not value dependent, so I just wanted to be certain that assertion wouldn't trigger. Sounds like it won't because this should only be called on an instantiation?

1755

Fantastic, thank you!

clang/lib/CodeGen/CGOpenMPRuntime.cpp
11087

Ah, cool -- slight preference for using "FIXME" to describe those sort of situations. TODO sounds like someone forgot to do it rather than someone recognizing it needs to be fixed.

clang/utils/TableGen/ClangAttrEmitter.cpp
110

I like it. :-)

jdoerfert updated this revision to Diff 244544.Feb 13 2020, 4:00 PM
jdoerfert marked 34 inline comments as done.

Addressed all comments by @aaron.ballman

jdoerfert added inline comments.Feb 13 2020, 4:05 PM
clang/lib/AST/OpenMPClause.cpp
1741

I added detection and tests for constant, value-dependent, and non-constant expressions in score and user condition. Assuming I didn't miss anything it should not assert here.

clang/lib/Parse/ParseOpenMP.cpp
1368–1377

I made sure all OMPTraitInfo are freed, I think. If an OMPDeclareVariantAttr is build its taking owenership and calls delete in the destructor.

clang/utils/TableGen/ClangAttrEmitter.cpp
110

It's gone now ;)

jdoerfert marked 11 inline comments as done.Feb 13 2020, 4:11 PM
jdoerfert added inline comments.
clang/utils/TableGen/ClangAttrEmitter.cpp
369

Overlooked this one, sorry. Will fix it and make it unreachable again.

jdoerfert updated this revision to Diff 244683.Feb 14 2020, 8:35 AM

Address last comment

aaron.ballman accepted this revision.Feb 14 2020, 9:16 AM

LGTM aside from some small nits.

clang/include/clang/Basic/Attr.td
186–192

Rather than describing the internal data structure form, I am hoping for comments that show the format of the user-facing pragma arguments. e.g., what information is in what position. Basically, something so I can mentally map from "OMPTraitsInfoArgument" to "oh, that's right, the arguments are specified in this order with these types" (roughly -- a general idea of the argument is all I'm looking for).

clang/lib/Parse/ParseOpenMP.cpp
1368–1377

That is reasonable enough -- any chance we can easily turn it into a std::unique_ptr<> and then get rid of the destructor entirely? I don't insist, but it would be cleaner, to my thinking.

This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Feb 15 2020, 12:05 PM

It looks like this may cause ASan failures http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/38749/steps/check-clang%20asan/logs/stdio

It would be great if you could take a look and possibly revert the change if it takes some time to investigate.

rnk added a subscriber: rnk.Mar 13 2020, 5:11 PM
rnk added inline comments.
clang/include/clang/AST/OpenMPClause.h
6682

This is not a good data structure choice. You have three levels of small vector nesting, so sizeof OMPTraitInfo is 880 bytes, and then you are passing it by value in in some of the attribute classes. Are you sure you wanted to do that?

jdoerfert marked an inline comment as done.Mar 13 2020, 5:44 PM
jdoerfert added inline comments.
clang/include/clang/AST/OpenMPClause.h
6682

I could make them all 0 elements long, reducing the size quite a bit for the common case of very few trait sets/selectors/properties. I could also try to dynamically allocate them once. WDYT?

jdoerfert marked an inline comment as done.Mar 13 2020, 11:42 PM
jdoerfert added inline comments.
clang/include/clang/AST/OpenMPClause.h
6682

One way of fixing this is D76173