[OpenACC} Improve diagnostics for 'tag's on clauses/directives

[erichkeane]

The 'cache' directive and various clauses have a 'tag' name that is optional. This patch cleans up the use of the 'cache' version so that we get a nicer diagnostic, and enables us to do the same with clauses in the same situation.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

The 'cache' directive and various clauses have a 'tag' name that is optional. This patch cleans up the use of the 'cache' version so that we get a nicer diagnostic, and enables us to do the same with clauses in the same situation.

---

Full diff: https://github.com/llvm/llvm-project/pull/77957.diff

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+2)
• (modified) clang/include/clang/Basic/OpenACCKinds.h (+156)
• (modified) clang/lib/Parse/ParseOpenACC.cpp (+49-13)
• (modified) clang/test/ParserOpenACC/parse-cache-construct.c (+12)

diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 088f8b74983c86..31530444d6920c 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1366,6 +1366,8 @@ def err_acc_invalid_open_paren
     : Error<"expected clause-list or newline in OpenACC directive">;
 def err_acc_invalid_default_clause_kind
     : Error<"invalid value for 'default' clause; expected 'present' or 'none'">;
+def err_acc_invalid_tag_kind
+    : Error<"invalid tag %0 on '%1' %select{directive|clause}2">;
 
 // OpenMP support.
 def warn_pragma_omp_ignored : Warning<
diff --git a/clang/include/clang/Basic/OpenACCKinds.h b/clang/include/clang/Basic/OpenACCKinds.h
index e860893b933ca3..a06db98fcba112 100644
--- a/clang/include/clang/Basic/OpenACCKinds.h
+++ b/clang/include/clang/Basic/OpenACCKinds.h
@@ -14,6 +14,9 @@
 #ifndef LLVM_CLANG_BASIC_OPENACCKINDS_H
 #define LLVM_CLANG_BASIC_OPENACCKINDS_H
 
+#include "clang/Basic/Diagnostic.h"
+#include "llvm/Support/ErrorHandling.h"
+
 namespace clang {
 // Represents the Construct/Directive kind of a pragma directive. Note the
 // OpenACC standard is inconsistent between calling these Construct vs
@@ -62,6 +65,76 @@ enum class OpenACCDirectiveKind {
   Invalid,
 };
 
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &Out,
+                                             OpenACCDirectiveKind K) {
+  switch (K) {
+  case OpenACCDirectiveKind::Parallel:
+    return Out << "parallel";
+
+  case OpenACCDirectiveKind::Serial:
+    return Out << "serial";
+
+  case OpenACCDirectiveKind::Kernels:
+    return Out << "kernels";
+
+  case OpenACCDirectiveKind::Data:
+    return Out << "data";
+
+  case OpenACCDirectiveKind::EnterData:
+    return Out << "enter data";
+
+  case OpenACCDirectiveKind::ExitData:
+    return Out << "exit data";
+
+  case OpenACCDirectiveKind::HostData:
+    return Out << "host_data";
+
+  case OpenACCDirectiveKind::Loop:
+    return Out << "loop";
+
+  case OpenACCDirectiveKind::Cache:
+    return Out << "cache";
+
+  case OpenACCDirectiveKind::ParallelLoop:
+    return Out << "parallel loop";
+
+  case OpenACCDirectiveKind::SerialLoop:
+    return Out << "serial loop";
+
+  case OpenACCDirectiveKind::KernelsLoop:
+    return Out << "kernels loop";
+
+  case OpenACCDirectiveKind::Atomic:
+    return Out << "atomic";
+
+  case OpenACCDirectiveKind::Declare:
+    return Out << "declare";
+
+  case OpenACCDirectiveKind::Init:
+    return Out << "init";
+
+  case OpenACCDirectiveKind::Shutdown:
+    return Out << "shutdown";
+
+  case OpenACCDirectiveKind::Set:
+    return Out << "set";
+
+  case OpenACCDirectiveKind::Update:
+    return Out << "update";
+
+  case OpenACCDirectiveKind::Wait:
+    return Out << "wait";
+
+  case OpenACCDirectiveKind::Routine:
+    return Out << "routine";
+
+  case OpenACCDirectiveKind::Invalid:
+    return Out << "<invalid>";
+
+  }
+  llvm_unreachable("Uncovered directive kind");
+}
+
 enum class OpenACCAtomicKind {
   Read,
   Write,
@@ -138,6 +211,89 @@ enum class OpenACCClauseKind {
   Invalid,
 };
 
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &Out,
+                                             OpenACCClauseKind K) {
+  switch (K) {
+  case OpenACCClauseKind::Finalize:
+    return Out << "finalize";
+
+  case OpenACCClauseKind::IfPresent:
+    return Out << "if_present";
+
+  case OpenACCClauseKind::Seq:
+    return Out << "seq";
+
+  case OpenACCClauseKind::Independent:
+    return Out << "independent";
+
+  case OpenACCClauseKind::Auto:
+    return Out << "auto";
+
+  case OpenACCClauseKind::Worker:
+    return Out << "worker";
+
+  case OpenACCClauseKind::Vector:
+    return Out << "vector";
+
+  case OpenACCClauseKind::NoHost:
+    return Out << "nohost";
+
+  case OpenACCClauseKind::Default:
+    return Out << "default";
+
+  case OpenACCClauseKind::If:
+    return Out << "if";
+
+  case OpenACCClauseKind::Self:
+    return Out << "self";
+
+  case OpenACCClauseKind::Copy:
+    return Out << "copy";
+
+  case OpenACCClauseKind::UseDevice:
+    return Out << "use_device";
+
+  case OpenACCClauseKind::Attach:
+    return Out << "attach";
+
+  case OpenACCClauseKind::Delete:
+    return Out << "delete";
+
+  case OpenACCClauseKind::Detach:
+    return Out << "detach";
+
+  case OpenACCClauseKind::Device:
+    return Out << "device";
+
+  case OpenACCClauseKind::DevicePtr:
+    return Out << "deviceptr";
+
+  case OpenACCClauseKind::DeviceResident:
+    return Out << "device_resident";
+
+  case OpenACCClauseKind::FirstPrivate:
+    return Out << "firstprivate";
+
+  case OpenACCClauseKind::Host:
+    return Out << "host";
+
+  case OpenACCClauseKind::Link:
+    return Out << "link";
+
+  case OpenACCClauseKind::NoCreate:
+    return Out << "no_create";
+
+  case OpenACCClauseKind::Present:
+    return Out << "present";
+
+  case OpenACCClauseKind::Private:
+    return Out << "private";
+
+  case OpenACCClauseKind::Invalid:
+    return Out << "<invalid>";
+  }
+  llvm_unreachable("Uncovered clause kind");
+}
 enum class OpenACCDefaultClauseKind {
   /// 'none' option.
   None,
diff --git a/clang/lib/Parse/ParseOpenACC.cpp b/clang/lib/Parse/ParseOpenACC.cpp
index 83378a094492b2..fb4364fa1dd8a7 100644
--- a/clang/lib/Parse/ParseOpenACC.cpp
+++ b/clang/lib/Parse/ParseOpenACC.cpp
@@ -164,6 +164,49 @@ bool isOpenACCSpecialToken(OpenACCSpecialTokenKind Kind, Token Tok) {
   llvm_unreachable("Unknown 'Kind' Passed");
 }
 
+// Used for cases where we have a token we want to check against an
+// 'identifier-like' token, but don't want to give awkward error messages in
+// cases where it is accidentially a keyword.
+bool isTokenIdentifierOrKeyword(Parser &P, Token Tok) {
+  if (Tok.is(tok::identifier))
+    return true;
+
+  if (!Tok.isAnnotation() && Tok.getIdentifierInfo() &&
+      Tok.getIdentifierInfo()->isKeyword(P.getLangOpts()))
+    return true;
+
+  return false;
+}
+
+
+/// Parses and consumes an identifer followed immediately by a single colon, and
+/// diagnoses if it is not the 'special token' kind that we require. Used when
+/// the tag is the only valid value.
+/// Return 'true' if the special token was matched, false if no special token,
+/// or an invalid special token was found.
+template <typename DirOrClauseTy>
+bool TryParseAndConsumeSpecialTokenKind(Parser &P, OpenACCSpecialTokenKind Kind,
+                                        DirOrClauseTy DirOrClause) {
+  Token IdentTok = P.getCurToken();
+  // If this is an identifier-like thing followed by ':', it is one of the
+  // OpenACC 'special' name tags, so consume it.
+  if (isTokenIdentifierOrKeyword(P, IdentTok) && P.NextToken().is(tok::colon)) {
+    P.ConsumeToken();
+    P.ConsumeToken();
+
+    if (!isOpenACCSpecialToken(Kind, IdentTok)) {
+      P.Diag(IdentTok, diag::err_acc_invalid_tag_kind)
+          << IdentTok.getIdentifierInfo() << DirOrClause
+          << std::is_same_v<DirOrClauseTy, OpenACCClauseKind>;
+      return false;
+    }
+
+    return true;
+  }
+
+  return false;
+}
+
 bool isOpenACCDirectiveKind(OpenACCDirectiveKind Kind, Token Tok) {
   if (!Tok.is(tok::identifier))
     return false;
@@ -218,11 +261,7 @@ bool isOpenACCDirectiveKind(OpenACCDirectiveKind Kind, Token Tok) {
 bool expectIdentifierOrKeyword(Parser &P) {
   Token Tok = P.getCurToken();
 
-  if (Tok.is(tok::identifier))
-    return false;
-
-  if (!Tok.isAnnotation() && Tok.getIdentifierInfo() &&
-      Tok.getIdentifierInfo()->isKeyword(P.getLangOpts()))
+  if (isTokenIdentifierOrKeyword(P, Tok))
     return false;
 
   P.Diag(P.getCurToken(), diag::err_expected) << tok::identifier;
@@ -674,14 +713,11 @@ void Parser::ParseOpenACCCacheVarList() {
     return;
 
   // The VarList is an optional `readonly:` followed by a list of a variable
-  // specifications.  First, see if we have `readonly:`, else we back-out and
-  // treat it like the beginning of a reference to a potentially-existing
-  // `readonly` variable.
-  if (isOpenACCSpecialToken(OpenACCSpecialTokenKind::ReadOnly, Tok) &&
-      NextToken().is(tok::colon)) {
-    // Consume both tokens.
-    ConsumeToken();
-    ConsumeToken();
+  // specifications. Consume something that looks like a 'tag', and diagnose if
+  // it isn't 'readonly'.
+  if (TryParseAndConsumeSpecialTokenKind(*this,
+                                         OpenACCSpecialTokenKind::ReadOnly,
+                                         OpenACCDirectiveKind::Cache)) {
     // FIXME: Record that this is a 'readonly' so that we can use that during
     // Sema/AST generation.
   }
diff --git a/clang/test/ParserOpenACC/parse-cache-construct.c b/clang/test/ParserOpenACC/parse-cache-construct.c
index d54632fc8f466a..093587f37df4fc 100644
--- a/clang/test/ParserOpenACC/parse-cache-construct.c
+++ b/clang/test/ParserOpenACC/parse-cache-construct.c
@@ -114,6 +114,18 @@ void func() {
     #pragma acc cache(readonly)
   }
 
+  for (int i = 0; i < 10; ++i) {
+    // expected-error@+2{{invalid tag 'devnum' on 'cache' directive}}
+    // expected-warning@+1{{OpenACC directives not yet implemented, pragma ignored}}
+    #pragma acc cache(devnum:ArrayPtr)
+  }
+
+  for (int i = 0; i < 10; ++i) {
+    // expected-error@+2{{invalid tag 'invalid' on 'cache' directive}}
+    // expected-warning@+1{{OpenACC directives not yet implemented, pragma ignored}}
+    #pragma acc cache(invalid:ArrayPtr)
+  }
+
   for (int i = 0; i < 10; ++i) {
     // expected-warning@+1{{OpenACC directives not yet implemented, pragma ignored}}
     #pragma acc cache(readonly:ArrayPtr)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[alexey-bataev]

LG with a nit

[erichkeane]

LG with a nit

Thanks for the super quick reviews!