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

[Erich Keane via Phabricator via Openmp-commits]

erichkeane 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)
----------------
aaron.ballman wrote:
> 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.
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.


================
Comment at: clang/lib/Basic/Attributes.cpp:26
+  // machinery that goes through TableGen.
+  if (LangOpts.OpenMP >= 51 && ScopeName == "omp")
+    return Name == "directive" || Name == "sequence";
----------------
aaron.ballman wrote:
> 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).
Ah!  I missed that this was that context.  Disregard :) 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105648