Page MenuHomePhabricator

[OpenMP] Support OpenMP 5.1 attributes
ClosedPublic

Authored by aaron.ballman on Jul 8 2021, 11:22 AM.

Details

Summary

OpenMP 5.1 added support for writing OpenMP directives using [[]] syntax in addition to using #pragma and this introduces support for the new syntax.

In OpenMP, the attributes take one of two forms: [[omp::directive(...)]] or [[omp::sequence(...)]]. A directive attribute contains an OpenMP directive clause that is identical to the analogous #pragma syntax. A sequence attribute can contain either sequence or directive arguments and is used to ensure that the attributes are processed sequentially for situations where the order of the attributes matter (remember: https://eel.is/c++draft/dcl.attr.grammar#4.sentence-4).

The approach taken here is somewhat novel and deserves mention. We could refactor much of the OpenMP parsing logic to work for either pragma annotation tokens or for attribute clauses. It would be a fair amount of effort to share the logic for both, but it's certainly doable. However, the semantic attribute system is not designed to handle the arbitrarily complex arguments that OpenMP directives contain. Adding support to thread the novel parsed information until we can produce a semantic attribute would be considerably more effort. What's more, existing OpenMP constructs are not (often) represented as semantic attributes. So doing this through Attr.td would be a massive undertaking that would likely only benefit OpenMP and comes with additional risks. Rather than walk down that path, I am taking advantage of the fact that the syntax of the directives within the directive clause is identical to that of the #pragma form. Once the parser recognizes that we're processing an OpenMP attribute, it caches all of the directive argument tokens and then replays them as though the user wrote a pragma. This reuses the same OpenMP parsing and semantic logic directly, but does come with a risk if the OpenMP committee decides to purposefully diverge their pragma and attribute syntaxes. So, despite this being a novel approach that does token replay, I think it's actually a better approach than trying to do this through the declarative syntax in Attr.td.

Diff Detail

Event Timeline

aaron.ballman created this revision.Jul 8 2021, 11:22 AM
aaron.ballman requested review of this revision.Jul 8 2021, 11:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 11:22 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
ABataev added inline comments.Jul 8 2021, 11:27 AM
clang/include/clang/Parse/Parser.h
2804–2808

I think better to commit this as NFC in a separate patch.

clang/lib/Parse/ParseDeclCXX.cpp
4173–4174
else {
  assert(AttrName->isStr("sequence"));

? Or we can expect something else here?

4177–4181

Common code between then and else.

erichkeane added inline comments.Jul 8 2021, 11:32 AM
clang/include/clang/Basic/TokenKinds.def
876

pragma_openmp_from_attr? Funny since it isn't a pragma...

clang/lib/Basic/Attributes.cpp
26

Should we diagnose in non-OMP5.1 uses of this?

clang/lib/Parse/ParseDeclCXX.cpp
4152–4156

Can this section be pulled outside of these? Both seem to have it in common.

4211

Is 'else' an unreachable'?

4281

Another place to make the same comment :D I wonder if giving a diagnostic on attempting to use omp::directive in a previous version should have either an extension warning or more explicit warning would be a good idea?

clang/lib/Parse/ParseStmt.cpp
109

Nit: This change is unrelated to the patch? Seems you have a couple of those (including the case changes of params above).

aaron.ballman marked 2 inline comments as done.Jul 8 2021, 11:49 AM
aaron.ballman added inline comments.
clang/include/clang/Basic/TokenKinds.def
876

All of our pragma annotation tokens start with pragma (and we generate a pragma annotation token to handle the parsing), so this is intended to convey that it's a pragma_openmp annotation token that comes from_attr.

But if you have a better name you'd prefer I use, I could certainly switch, but I think we want to keep pragma_openmp as the prefix to make it clear that this is paired with the preceding pragma_openmp annotation token.

clang/include/clang/Parse/Parser.h
2804–2808

Yeah, I can pull this out -- I had some intermediary changes there, but removed them and forgot to revert the renaming.

clang/lib/Basic/Attributes.cpp
26

What would we diagnose here? This is __has_cpp_attribute's internal implementation, so I don't think we want a diagnostic here. I asked Mike Rice whether he thought we should support this syntax in older OpenMP modes and his recommendation was to not do that yet (I guess it's less common to backport features to older OpenMP standard modes).

clang/lib/Parse/ParseDeclCXX.cpp
4152–4156

Sure -- I went back and forth on whether to do that. :-D

4173–4174

Oh, yeah, we can do that given that the hasAttribute() check works to filter out unknown attributes. Good call!

4211

Yes. I'm going to assert that based on the comment from @ABataev

4281

Ah, now I see what you're after. We could do that, but I think deciding whether to expose this in older modes is more of an open question. I went with the conservative approach first, but we could relax this later.

clang/lib/Parse/ParseStmt.cpp
109

Yeah, I'll back that out.

erichkeane added inline comments.Jul 8 2021, 11:55 AM
clang/include/clang/Basic/TokenKinds.def
876

I guess I can buy that... it ends up being a little disorienting to see that... to me pragma_ meant #pragma, but I guess it could mean PRAGMA_ANNOTATION(.

That said, I would think PRAGMA_ANNOTATION is represented by the annot the macro adds...

I guess I would lean towards this being openmp_attr, but I don't feel strongly about that.

clang/lib/Basic/Attributes.cpp
26

Ah! I missed that this was that context. Disregard :)

clang/lib/Parse/ParseDeclCXX.cpp
4281

Well, I was going for 1 of two things:

1- allow in older modes, then do a warning that you're using it in the wrong version.

2- Customize the error from "unknown attribute directive" to something like, "attribute omp::directive is only available in OMP5.1 mode".

ABataev added inline comments.Jul 8 2021, 12:15 PM
clang/lib/Parse/ParseDeclCXX.cpp
4281

I think we can enable it for the previous versions, at least as an extension. Thoughts?

erichkeane added inline comments.Jul 8 2021, 12:26 PM
clang/lib/Parse/ParseDeclCXX.cpp
4281

It'll mean a larger test-surface I'd think, but there _IS_ precedent both ways.
IMO, we are probably better off doing #2 above (a better 'unknown attribute' diagnostic), then doing it as an extension if GCC makes that choice, or there is a compelling reaosn.

aaron.ballman marked 8 inline comments as done.

Addressing review feedback

clang/lib/Parse/ParseDeclCXX.cpp
4474–4486

I'm also backing out these NFC formatting changes that we vestiges from earlier work.

aaron.ballman added inline comments.Jul 8 2021, 12:53 PM
clang/lib/Parse/ParseDeclCXX.cpp
4281

I think it makes sense to improve the diagnostic here, but I think we should hold off on enabling it as an extension in older OpenMP modes until we have some usage experience with the feature. Once we have confidence that the feature works well in 5.1, then it's easy enough for us to support it in older modes.

Looks good, in general, just pre-commit formatting changes.

clang/lib/Basic/Attributes.cpp
27

Better to return int value here directly, just in case

aaron.ballman added inline comments.Jul 8 2021, 1:17 PM
clang/lib/Parse/ParseDeclCXX.cpp
4281

Actually, I'm rethinking whether that makes sense. In order to have the new diagnostic, we'd have to perform this check *before* the hasAttribute() check above because the attributes aren't known. That adds a bit more checking back into the patch that was just removed (not the end of the world) and it makes out-of-spec OpenMP attributes behave differently than other out-of-spec kinds of attributes (which is strange). I think my preference is to leave this with the generic "unknown attribute" diagnostic. If we decide to enable it as an extension, then hasAttribute() will know about them and we can more easily add the forward/backward compat warnings at that point. WDYT?

aaron.ballman marked 2 inline comments as done.

Address review feedback, add a link to this review to the OpenMP status page for the feature.

erichkeane added inline comments.Jul 8 2021, 2:43 PM
clang/lib/Parse/ParseDeclCXX.cpp
4281

I guess my main motivation is that OMP version is a little bit more of a subtle difference, so I'd think the hint would be appreciated. That said, it DOES look like it is going to be enough of a hassle that we could perhaps do it as a future patch.

There is perhaps an improvement todiagnostics in general here (for the Attribute owner, not the author of this patch :-P) to differentiate, "this attribute name is nonsense" vs "this attribute name is only available in higher language settings".

cor3ntin added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
2670–2676

The comment raises 2 questions: should it be called annot_openmp_attr instead? Does the comment describe what this does?
I imagine long terms attributes might be the ""normal"" scenario?

jdoerfert added inline comments.Jul 8 2021, 2:47 PM
clang/lib/Parse/ParseDeclCXX.cpp
2670–2676

I imagine long terms attributes might be the ""normal"" scenario?

We can't assume that (C) and for C++ only time will tell.

erichkeane added inline comments.Jul 8 2021, 3:50 PM
clang/lib/Parse/ParseDeclCXX.cpp
2670–2676

FWIW, C23 is getting C++ style attributes as well, so we MIGHT be able to make that assumption some day :D

That said, the fact that these are called "PRAGMA_ANNOTATION" in TokenKinds.def seems misnamed to me anymore, which I think is the confusion. It is a little strange that the annot is added automatically, but the pragma isn't...

Either way, I think it is debatable what the pragma in these relates to. My opinion is that it applies to the syntax (since the rest are #pragma SOMETHING), not that it is a PRAGMA_ANNOTATION, and I liked annot_attr_openmp to match annot_pragma_openmp, but I don't feel terribly strongly. See our conversation on TokenKinds for the other half of this discussion.

aaron.ballman marked an inline comment as done.Jul 9 2021, 4:39 AM
aaron.ballman added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
2670–2676

FWIW, C23 is getting C++ style attributes as well, so we MIGHT be able to make that assumption some day :D

And FWIW, I'm explicitly supporting OpenMP 5.1 attributes when -fdouble-square-bracket-attributes is enabled along with OpenMP 5.1 (which includes C23 mode by default). I just noticed that the OpenMP spec doesn't require this support in C, so should I remove that support in this patch (we can enable it as an extension when we allow it in older OpenMP modes), should I diagnose this as an extension only in C mode, or should I enable this as an extension in all OpenMP modes and add diagnostics for it?

That said, the fact that these are called "PRAGMA_ANNOTATION" in TokenKinds.def seems misnamed to me anymore, which I think is the confusion. It is a little strange that the annot is added automatically, but the pragma isn't...

The fact that at least two people have found the name and definition of the token confusing means I'll be changing it. I think ANNOTATION(attr_openmp) will actually work out fine. The only use of the PRAGMA_ANNOTATION macro is in the definition of tok::isPragmaAnnotation() and the only places we call that function are places we're already looking for tok::annot_pragma_openmp_from_attr explicitly prior to the call anyway. The one oddity to this is that it starts with an annot_attr_openmp token and ends with an annot_pragma_openmp_end token -- but I don't think that should cause massive confusion (the end token is only interesting to the OpenMP parsing methods and those are designed around pragma terminology anyway).

4281

There is perhaps an improvement todiagnostics in general here (for the Attribute owner, not the author of this patch :-P) to differentiate, "this attribute name is nonsense" vs "this attribute name is only available in higher language settings".

Agreed -- we have some wiggle room for improvement here (Attr.td allows you to specify attributes by language option and target and we should try to be consistent with diagnosing "unknown" attributes there as well).

aaron.ballman marked an inline comment as done.

Change annot_pragma_openmp_from_attr into annot_attr_openmp based on review feedback.

ABataev added inline comments.Jul 9 2021, 4:43 AM
clang/lib/Parse/ParseDeclCXX.cpp
2670–2676

And FWIW, I'm explicitly supporting OpenMP 5.1 attributes when -fdouble-square-bracket-attributes is enabled along with OpenMP 5.1 (which includes C23 mode by default). I just noticed that the OpenMP spec doesn't require this support in C, so should I remove that support in this patch (we can enable it as an extension when we allow it in older OpenMP modes), should I diagnose this as an extension only in C mode, or should I enable this as an extension in all OpenMP modes and add diagnostics for it?

I would support all this stuff as an extension and emit a warning/note for the old versions, possibly ignored by default.

aaron.ballman marked 8 inline comments as done.Jul 9 2021, 4:50 AM
aaron.ballman added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
2670–2676

Okay, I can do that.

Enable attribute support as an extension (with the usual diagnostics). Note that this adds some new diagnostic groups for OpenMP as well.

aaron.ballman marked an inline comment as done.Jul 9 2021, 5:26 AM
ABataev accepted this revision.Jul 9 2021, 5:33 AM

LG, but wait for others' comments/suggestions/etc.

This revision is now accepted and ready to land.Jul 9 2021, 5:33 AM

LG, but wait for others' comments/suggestions/etc.

Thank you for the review!

erichkeane accepted this revision.Jul 9 2021, 5:59 AM
cor3ntin accepted this revision.Jul 9 2021, 6:08 AM

Thanks for changing the name. It looks good!

aaron.ballman closed this revision.Jul 12 2021, 3:54 AM

Thank you for the reviews, everyone. I've commit in de59f564400de1b0fe30ae07f3c800562a025e27.