default clause replaced by otherwise clause for metadirective in OpenMP 5.2 #128640

ravurvi20 · 2025-02-25T06:55:21Z

This PR replaces the default clause with the otherwise clause for the metadirective in OpenMP. The otherwise clause serves as a fallback condition when no directive from the when clauses is selected. In the when clause, context selectors define traits evaluated to determine the directive to be applied.
The previous merge was reverted due to a failing test case, which has now been resolved.

llvmbot · 2025-02-25T06:55:55Z

@llvm/pr-subscribers-clang

Author: Urvi Rav (ravurvi20)

Changes

This PR replaces the default clause with the otherwise clause for the metadirective in OpenMP. The otherwise clause serves as a fallback condition when no directive from the when clauses is selected. In the when clause, context selectors define traits evaluated to determine the directive to be applied.
The previous merge was reverted due to a failing test case, which has now been resolved.

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

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+4)
(modified) clang/lib/Parse/ParseOpenMP.cpp (+20)
(modified) clang/test/OpenMP/metadirective_messages.cpp (+44-17)

diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index c513dab810d1f..4b8449e9ee9b6 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1657,6 +1657,10 @@ def err_omp_expected_colon : Error<"missing ':' in %0">;
 def err_omp_missing_comma : Error< "missing ',' after %0">;
 def err_omp_expected_context_selector
     : Error<"expected valid context selector in %0">;
+def err_omp_unknown_clause
+    : Error<"unknown clause '%0' in %1">;
+def warn_omp_default_deprecated : Warning<"'default' clause for"
+  " 'metadirective' is deprecated; use 'otherwise' instead">, InGroup<Deprecated>;
 def err_omp_requires_out_inout_depend_type : Error<
   "reserved locator 'omp_all_memory' requires 'out' or 'inout' "
   "dependency types">;
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index 42e6aac681c1c..3b86847e937a2 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -2759,6 +2759,19 @@ StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective(
       OpenMPClauseKind CKind = Tok.isAnnotation()
                                    ? OMPC_unknown
                                    : getOpenMPClauseKind(PP.getSpelling(Tok));
+      // Check if the clause is unrecognized.
+      if (getLangOpts().OpenMP < 52 &&
+          (CKind == OMPC_unknown || CKind == OMPC_otherwise)) {
+        Diag(Tok, diag::err_omp_unknown_clause)
+            << PP.getSpelling(Tok) << "metadirective";
+      }
+      if (getLangOpts().OpenMP >= 52 && CKind == OMPC_unknown) {
+        Diag(Tok, diag::err_omp_unknown_clause)
+            << PP.getSpelling(Tok) << "metadirective";
+      }
+      if (CKind == OMPC_default && getLangOpts().OpenMP >= 52) {
+        Diag(Tok, diag::warn_omp_default_deprecated);
+      }
       SourceLocation Loc = ConsumeToken();
 
       // Parse '('.
@@ -2785,6 +2798,13 @@ StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective(
           return Directive;
         }
       }
+      if (CKind == OMPC_otherwise) {
+        // Check for 'otherwise' keyword.
+        if (Tok.is(tok::identifier) &&
+            Tok.getIdentifierInfo()->getName() == "otherwise") {
+          ConsumeToken(); // Consume 'otherwise'
+        }
+      }
       // Skip Directive for now. We will parse directive in the second iteration
       int paren = 0;
       while (Tok.isNot(tok::r_paren) || paren != 0) {
diff --git a/clang/test/OpenMP/metadirective_messages.cpp b/clang/test/OpenMP/metadirective_messages.cpp
index 7fce9fa446058..40ea37845fdff 100644
--- a/clang/test/OpenMP/metadirective_messages.cpp
+++ b/clang/test/OpenMP/metadirective_messages.cpp
@@ -2,21 +2,48 @@
 
 // RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -verify -fopenmp-simd -x c++ -std=c++14 -fexceptions -fcxx-exceptions %s
 
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -verify=expected,omp52 -fopenmp -fopenmp-version=52 -ferror-limit 100 -o - %s -Wuninitialized
+
 void foo() {
-#pragma omp metadirective // expected-error {{expected expression}}
-  ;
-#pragma omp metadirective when() // expected-error {{expected valid context selector in when clause}} expected-error {{expected expression}} expected-warning {{expected identifier or string literal describing a context set; set skipped}} expected-note {{context set options are: 'construct' 'device' 'target_device' 'implementation' 'user'}} expected-note {{the ignored set spans until here}}
-  ;
-#pragma omp metadirective when(device{}) // expected-warning {{expected '=' after the context set name "device"; '=' assumed}} expected-warning {{expected identifier or string literal describing a context selector; selector skipped}} expected-note {{context selector options are: 'kind' 'arch' 'isa'}} expected-note {{the ignored selector spans until here}} expected-error {{expected valid context selector in when clause}} expected-error {{expected expression}}
-  ;
-#pragma omp metadirective when(device{arch(nvptx)}) // expected-error {{missing ':' in when clause}} expected-error {{expected expression}} expected-warning {{expected '=' after the context set name "device"; '=' assumed}}
-  ;
-#pragma omp metadirective when(device{arch(nvptx)}: ) default() // expected-warning {{expected '=' after the context set name "device"; '=' assumed}}
-  ;
-#pragma omp metadirective when(device = {arch(nvptx)} : ) default(xyz) // expected-error {{expected an OpenMP directive}} expected-error {{use of undeclared identifier 'xyz'}}
-  ;
-#pragma omp metadirective when(device = {arch(nvptx)} : parallel default() // expected-error {{expected ',' or ')' in 'when' clause}} expected-error {{expected expression}}
-  ;
-#pragma omp metadirective when(device = {isa("some-unsupported-feature")} : parallel) default(single) // expected-warning {{isa trait 'some-unsupported-feature' is not known to the current target; verify the spelling or consider restricting the context selector with the 'arch' selector further}}
-  ;
-}
+  #if _OPENMP >= 202111
+    #pragma omp metadirective // omp52-error {{expected expression}}
+      ;
+    #pragma omp metadirective when() // omp52-error {{expected valid context selector in when clause}} expected-error {{expected expression}} expected-warning {{expected identifier or string literal describing a context set; set skipped}} expected-note {{context set options are: 'construct' 'device' 'target_device' 'implementation' 'user'}} expected-note {{the ignored set spans until here}}
+      ;
+    #pragma omp metadirective when(device{}) // omp52-warning {{expected '=' after the context set name "device"; '=' assumed}} expected-warning {{expected identifier or string literal describing a context selector; selector skipped}} expected-note {{context selector options are: 'kind' 'arch' 'isa'}} expected-note {{the ignored selector spans until here}} expected-error {{expected valid context selector in when clause}} expected-error {{expected expression}}
+      ;
+    #pragma omp metadirective when(device{arch(nvptx)}) // omp52-error {{missing ':' in when clause}} expected-error {{expected expression}} expected-warning {{expected '=' after the context set name "device"; '=' assumed}}
+      ;
+    #pragma omp metadirective when(device{arch(nvptx)}: ) otherwise() // omp52-warning {{expected '=' after the context set name "device"; '=' assumed}}
+      ;
+    #pragma omp metadirective when(device = {arch(nvptx)} : ) otherwise(xyz) // omp52-error {{expected an OpenMP directive}} expected-error {{use of undeclared identifier 'xyz'}}
+      ;
+    #pragma omp metadirective when(device = {arch(nvptx)} : parallel otherwise() // omp52-error {{expected ',' or ')' in 'when' clause}} expected-error {{expected expression}}
+      ;
+    #pragma omp metadirective when(device = {isa("some-unsupported-feature")} : parallel) otherwise(single) // omp52-warning {{isa trait 'some-unsupported-feature' is not known to the current target; verify the spelling or consider restricting the context selector with the 'arch' selector further}}
+      ;
+    #pragma omp metadirective when(device = {arch(nvptx)} : parallel) default() // omp52-warning {{'default' clause for 'metadirective' is deprecated; use 'otherwise' instead}}
+      ;
+    #pragma omp metadirective when(device = {arch(nvptx)} : parallel) xyz() //omp52-error {{unknown clause 'xyz' in metadirective}} 
+      ;
+  #else
+    #pragma omp metadirective // expected-error {{expected expression}}
+      ;
+    #pragma omp metadirective when() // expected-error {{expected valid context selector in when clause}} expected-error {{expected expression}} expected-warning {{expected identifier or string literal describing a context set; set skipped}} expected-note {{context set options are: 'construct' 'device' 'target_device' 'implementation' 'user'}} expected-note {{the ignored set spans until here}}
+      ;
+    #pragma omp metadirective when(device{}) // expected-warning {{expected '=' after the context set name "device"; '=' assumed}} expected-warning {{expected identifier or string literal describing a context selector; selector skipped}} expected-note {{context selector options are: 'kind' 'arch' 'isa'}} expected-note {{the ignored selector spans until here}} expected-error {{expected valid context selector in when clause}} expected-error {{expected expression}}
+      ;
+    #pragma omp metadirective when(device{arch(nvptx)}) // expected-error {{missing ':' in when clause}} expected-error {{expected expression}} expected-warning {{expected '=' after the context set name "device"; '=' assumed}}
+      ;
+    #pragma omp metadirective when(device{arch(nvptx)}: ) default() // expected-warning {{expected '=' after the context set name "device"; '=' assumed}}
+      ;
+    #pragma omp metadirective when(device = {arch(nvptx)} : ) default(xyz) // expected-error {{expected an OpenMP directive}} expected-error {{use of undeclared identifier 'xyz'}}
+      ;
+    #pragma omp metadirective when(device = {arch(nvptx)} : parallel default() // expected-error {{expected ',' or ')' in 'when' clause}} expected-error {{expected expression}}
+      ;
+    #pragma omp metadirective when(device = {isa("some-unsupported-feature")} : parallel) default(single) // expected-warning {{isa trait 'some-unsupported-feature' is not known to the current target; verify the spelling or consider restricting the context selector with the 'arch' selector further}}
+      ;
+    #pragma omp metadirective when(device = {arch(nvptx)} : parallel) xyz() //expected-error {{unknown clause 'xyz' in metadirective}} 
+      ;
+  #endif
+  }
\ No newline at end of file

ravurvi20 · 2025-02-28T06:40:37Z

Previous commit was reverted due to a failing test case, which has been resolved now. Parsing changes have been approved in the previous PR.
125648

ravurvi20 · 2025-03-24T09:21:22Z

@mjklemm could you please approve this PR. There was minor change which has been fixed now.

mjklemm

LGTM

kparzysz · 2025-03-24T17:06:40Z

clang/include/clang/Basic/DiagnosticParseKinds.td

+def err_omp_unknown_clause
+    : Error<"unknown clause '%0' in %1">;


We already have a message for a wrong clause for a directive: err_omp_unexpected_clause. This does not apply when we have some unknown string where a clause is expected.

kparzysz · 2025-03-24T17:15:43Z

clang/lib/Parse/ParseOpenMP.cpp

+      if (getLangOpts().OpenMP < 52 &&
+          (CKind == OMPC_unknown || CKind == OMPC_otherwise)) {
+        Diag(Tok, diag::err_omp_unknown_clause)
+            << PP.getSpelling(Tok) << "metadirective";
+      }
+      if (getLangOpts().OpenMP >= 52 && CKind == OMPC_unknown) {
+        Diag(Tok, diag::err_omp_unknown_clause)
+            << PP.getSpelling(Tok) << "metadirective";
+      }


Please treat OMPC_unknown and OMPC_otherwise separately: in the former case the message should be something like "expected an OpenMP clause" (see err_omp_unknown_directive for reference). The latter case should use err_omp_unexpected_clause.

@kparzysz I have made the requested modifications to the code based on your suggestions. Please review it and provide your feedback.

ping @kparzysz

github-actions · 2025-04-02T05:40:55Z

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

ravurvi20 · 2025-04-24T09:34:05Z

@alexey-bataev I have pushed the requested changes. Could you please approve this PR.

alexey-bataev · 2025-04-24T15:50:48Z

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1660,6 +1660,10 @@ def err_omp_expected_colon : Error<"missing ':' in %0">;
 def err_omp_missing_comma : Error< "missing ',' after %0">;
 def err_omp_expected_context_selector
    : Error<"expected valid context selector in %0">;
+def err_omp_unknown_clause
+    : Error<"expected an OpenMP clause">;
+def warn_omp_default_deprecated : Warning<"'default' clause for"


Use err_omp_deprecate_old_syntax

Since default is being deprecated not disallowed in metadirective I had put a warning instead on using an error (err_omp_deprecate_old_syntax).

alexey-bataev · 2025-04-24T15:52:49Z

clang/lib/Parse/ParseOpenMP.cpp

+      if (CKind == OMPC_default && getLangOpts().OpenMP >= 52) {
+        Diag(Tok, diag::warn_omp_default_deprecated);
+      }


Drop braces

alexey-bataev · 2025-04-24T15:53:49Z

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1660,6 +1660,10 @@ def err_omp_expected_colon : Error<"missing ':' in %0">;
 def err_omp_missing_comma : Error< "missing ',' after %0">;
 def err_omp_expected_context_selector
    : Error<"expected valid context selector in %0">;
+def err_omp_unknown_clause


Use err_omp_expected_clause instead

Thanks for the suggestion. I've already used err_omp_unexpected_clause for the OMPC_otherwise case in OpenMP versions prior to 5.2, as suggested earlier. For OMPC_unknown, I'm using err_omp_unknown_clause, which aligns with the guidance to treat OMPC_unknown and OMPC_otherwise separately.

It is wrong. If the wrong clause is used, it should be reported automatically. If none was specified, err_omp_expected_clause should be used

alexey-bataev · 2025-04-24T15:54:06Z

clang/lib/Parse/ParseOpenMP.cpp

+            Tok.getIdentifierInfo()->getName() == "otherwise") {
+          ConsumeToken(); // Consume 'otherwise'
+        }


Drop braces

alexey-bataev · 2025-04-24T15:54:43Z

clang/lib/Parse/ParseOpenMP.cpp

+        if (Tok.is(tok::identifier) &&
+            Tok.getIdentifierInfo()->getName() == "otherwise") {


Why need to double check here?

alexey-bataev · 2025-04-24T15:55:02Z

clang/test/OpenMP/metadirective_ast_print.c

-			       : parallel for) otherwise()
-  for (int i=0; i<10; i++)
-    ;
+


Why moved the test cases? and why a guard is required?

Since for versions prior to 5.2 default is valid so moved it in a way that if the version is 5.2 or greater otherwise condition should run else the default condition.

alexey-bataev · 2025-04-25T14:30:46Z

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1660,6 +1660,10 @@ def err_omp_expected_colon : Error<"missing ':' in %0">;
 def err_omp_missing_comma : Error< "missing ',' after %0">;
 def err_omp_expected_context_selector
    : Error<"expected valid context selector in %0">;
+def err_omp_unknown_clause


It is wrong. If the wrong clause is used, it should be reported automatically. If none was specified, err_omp_expected_clause should be used

alexey-bataev · 2025-04-25T14:31:10Z

clang/lib/Parse/ParseOpenMP.cpp

+      // Check if the clause is unrecognized.
+      if (CKind == OMPC_unknown) {
+        Diag(Tok, diag::err_omp_unknown_clause)
+            << PP.getSpelling(Tok) << "metadirective";


Error message does not accept any parameters

default clause replaced by otherwise clause for metadirective

6ebd599

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang labels Feb 25, 2025

mjklemm requested review from alexey-bataev, jdoerfert and kparzysz March 24, 2025 16:51

mjklemm approved these changes Mar 24, 2025

View reviewed changes

kparzysz reviewed Mar 24, 2025

View reviewed changes

ravurvi20 and others added 4 commits March 24, 2025 23:15

Merge branch 'llvm:main' into otherwise-clause

5efa687

Merge branch 'llvm:main' into otherwise-clause

a3a8a41

Merge branch 'llvm:main' into otherwise-clause

185a9ad

Update ParseOpenMP.cpp and error messages

24fe1cf

Update LIT test cases

44a2539

alexey-bataev reviewed Apr 24, 2025

View reviewed changes

Updated ParseOpenMP.cpp

f1b974c

ravurvi20 force-pushed the otherwise-clause branch from 0840851 to f1b974c Compare April 25, 2025 09:09

ravurvi20 requested review from kparzysz and alexey-bataev April 25, 2025 09:47

alexey-bataev reviewed Apr 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default clause replaced by otherwise clause for metadirective in OpenMP 5.2 #128640

default clause replaced by otherwise clause for metadirective in OpenMP 5.2 #128640

ravurvi20 commented Feb 25, 2025

llvmbot commented Feb 25, 2025

ravurvi20 commented Feb 28, 2025

ravurvi20 commented Mar 24, 2025

mjklemm left a comment

kparzysz Mar 24, 2025

kparzysz Mar 24, 2025

ravurvi20 Apr 2, 2025

ravurvi20 Apr 3, 2025

ravurvi20 Apr 14, 2025

github-actions bot commented Apr 2, 2025 •

edited

Loading

ravurvi20 commented Apr 24, 2025

alexey-bataev Apr 24, 2025

ravurvi20 Apr 25, 2025

alexey-bataev Apr 24, 2025

alexey-bataev Apr 24, 2025

ravurvi20 Apr 25, 2025

alexey-bataev Apr 25, 2025

alexey-bataev Apr 24, 2025

alexey-bataev Apr 24, 2025

alexey-bataev Apr 24, 2025

ravurvi20 Apr 25, 2025

alexey-bataev Apr 25, 2025

alexey-bataev Apr 25, 2025

		def err_omp_unknown_clause
		: Error<"unknown clause '%0' in %1">;

		if (Tok.is(tok::identifier) &&
		Tok.getIdentifierInfo()->getName() == "otherwise") {

default clause replaced by otherwise clause for metadirective in OpenMP 5.2 #128640

Are you sure you want to change the base?

default clause replaced by otherwise clause for metadirective in OpenMP 5.2 #128640

Conversation

ravurvi20 commented Feb 25, 2025

llvmbot commented Feb 25, 2025

ravurvi20 commented Feb 28, 2025

ravurvi20 commented Mar 24, 2025

mjklemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 2, 2025 • edited Loading

ravurvi20 commented Apr 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 2, 2025 •

edited

Loading