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

Fri Jul 9 04:39:16 PDT 2021

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.

================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2670-2676
+
+  // The next token may be an OpenMP pragma annotation token. That would
+  // normally be handled from ParseCXXClassMemberDeclarationWithPragmas, but in
+  // this case, it came from an *attribute* rather than a pragma. Handle it now.
+  if (Tok.is(tok::annot_pragma_openmp_from_attr))
+    return ParseOpenMPDeclarativeDirectiveWithExtDecl(AS, attrs);
+
----------------
erichkeane wrote:
> jdoerfert wrote:
> > cor3ntin wrote:
> > > 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?
> > > I imagine long terms attributes might be the ""normal"" scenario?
> > 
> > We can't assume that (C) and for C++ only time will tell.
> 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.
> 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).

================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4285
+    // created for us by the caller.
+    return true;
+  }
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > ABataev wrote:
> > > > > erichkeane wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 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.
> > > > > > 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".
> > > > > I think we can enable it for the previous versions, at least as an extension. Thoughts?
> > > > 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.
> > > 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.
> > 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?
> 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".
> 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).

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

https://reviews.llvm.org/D105648