[Openmp-commits] [PATCH] D105648: [OpenMP] Support OpenMP 5.1 attributes

[Aaron Ballman via Phabricator via Openmp-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/TokenKinds.def:869
 PRAGMA_ANNOTATION(pragma_openmp)
+PRAGMA_ANNOTATION(pragma_openmp_from_attr)
 PRAGMA_ANNOTATION(pragma_openmp_end)
----------------
erichkeane wrote:
> `pragma_openmp_from_attr`?  Funny since it isn't a pragma...
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.


================
Comment at: clang/include/clang/Parse/Parser.h:2803-2808
+  bool MaybeParseCXX11Attributes(ParsedAttributesWithRange &Attrs,
+                                 SourceLocation *EndLoc = nullptr,
                                  bool OuterMightBeMessageSend = false) {
     if (standardAttributesAllowed() &&
         isCXX11AttributeSpecifier(false, OuterMightBeMessageSend)) {
+      ParseCXX11Attributes(Attrs, EndLoc);
----------------
ABataev wrote:
> I think better to commit this as NFC in a separate patch.
Yeah, I can pull this out -- I had some intermediary changes there, but removed them and forgot to revert the renaming.


================
Comment at: clang/lib/Basic/Attributes.cpp:26
+  // machinery that goes through TableGen.
+  if (LangOpts.OpenMP >= 51 && ScopeName == "omp")
+    return Name == "directive" || Name == "sequence";
----------------
erichkeane wrote:
> Should we diagnose in non-OMP5.1 uses of this?
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).


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4152-4156
+    BalancedDelimiterTracker T(*this, tok::l_paren);
+    if (T.consumeOpen()) {
+      Diag(Tok, diag::err_expected) << tok::l_paren;
+      return;
+    }
----------------
erichkeane wrote:
> Can this section be pulled outside of these?  Both seem to have it in common.
Sure -- I went back and forth on whether to do that. :-D


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4173-4174
+    T.consumeClose();
+  } else if (AttrName->isStr("sequence")) {
+    // If the attribute is named 'sequence', its argument is a list of one or
+    // more OpenMP attributes (either 'omp::directive' or 'omp::sequence',
----------------
ABataev wrote:
> ```
> else {
>   assert(AttrName->isStr("sequence"));
> ```
> ? Or we can expect something else here?
Oh, yeah, we can do that given that the `hasAttribute()` check works to filter out unknown attributes. Good call!


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4211
+    T.consumeClose();
+  }
+}
----------------
erichkeane wrote:
> Is 'else' an unreachable'?
Yes. I'm going to assert that based on the comment from @ABataev 


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4285
+    // created for us by the caller.
+    return true;
+  }
----------------
erichkeane wrote:
> 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?
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.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:109
   ParsedAttributesWithRange Attrs(AttrFactory);
-  MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
+  MaybeParseCXX11Attributes(Attrs, nullptr,
+                            /*MightBeObjCMessageSend*/ true);
----------------
erichkeane wrote:
> Nit: This change is unrelated to the patch?  Seems you have a couple of those (including the case changes of params above).
Yeah, I'll back that out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105648/new/

https://reviews.llvm.org/D105648