[OpenMP] Missing implicit otherwise clause in metadirective. #127113

zahiraam · 2025-02-13T19:26:45Z

Compiling this:
int main() {
#pragma omp metadirective when(use r= {condition(0)}
: parallel for)
for (int i=0; i<10; i++)
;
}`

is generating an error:
error: expected expression
The compiler is interpreting this as if it's compiling a #pragma omp metadirective with no otherwise clause.
In the OMP5.2 specs chapter 7.4 it's mentioned that:
If no otherwise clause is specified the effect is as if one was specified without an associated directive variant.
This patch fixes the issue.

llvmbot · 2025-02-14T12:30:45Z

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-clang

Author: Zahira Ammarguellat (zahiraam)

Changes

Compiling this:
int main() {
#pragma omp metadirective when(use r= {condition(0)}
: parallel for)
for (int i=0; i<10; i++)
;
}`

is generating an error:
error: expected expression
The compiler is interpreting this as if it's compiling a #pragma omp metadirective with no otherwise clause.
In the OMP5.2 specs chapter 7.4 it's mentioned that:
If no otherwise clause is specified the effect is as if one was specified without an associated directive variant.
This patch fixes the issue.

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

4 Files Affected:

(modified) clang/lib/Parse/ParseOpenMP.cpp (+6)
(modified) clang/test/OpenMP/metadirective_ast_print.c (+18)
(added) clang/test/OpenMP/metadirective_otherwise.cpp (+72)
(modified) llvm/include/llvm/Frontend/OpenMP/OMPContext.h (+1-1)

diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index a455659ca8f2c..0f68d4002ad8e 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -2882,6 +2882,12 @@ StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective(
           /*ReadDirectiveWithinMetadirective=*/true);
       break;
     }
+    // If no match is found and no otherwise clause is present, skip
+    // OMP5.2 Chapter 7.4: If no otherwise clause is specified the effect is as
+    // if one was specified without an associated directive variant.
+    if (BestIdx == -1 && Idx == 1) {
+      SkipUntil(tok::annot_pragma_openmp_end);
+    }
     break;
   }
   case OMPD_threadprivate: {
diff --git a/clang/test/OpenMP/metadirective_ast_print.c b/clang/test/OpenMP/metadirective_ast_print.c
index d9ff7e7645216..06991c6f06a02 100644
--- a/clang/test/OpenMP/metadirective_ast_print.c
+++ b/clang/test/OpenMP/metadirective_ast_print.c
@@ -77,6 +77,24 @@ void foo(void) {
                                : parallel) default(nothing)
   for (int i = 0; i < 16; i++)
     ;
+
+#pragma omp metadirective when(user = {condition(0)}	\
+			       : parallel for) otherwise()
+  for (int i=0; i<10; i++)
+    ;
+#pragma omp metadirective when(user = {condition(0)}	\
+			       : parallel for)
+  for (int i=0; i<10; i++)
+    ;
+
+#pragma omp metadirective when(user = {condition(1)}	\
+			       : parallel for) otherwise()
+  for (int i=0; i<10; i++)
+    ;
+#pragma omp metadirective when(user = {condition(1)}	\
+			       : parallel for)
+  for (int i=0; i<10; i++)
+    ;
 }
 
 // CHECK: void bar(void);
diff --git a/clang/test/OpenMP/metadirective_otherwise.cpp b/clang/test/OpenMP/metadirective_otherwise.cpp
new file mode 100644
index 0000000000000..50c9fe8a2b0e7
--- /dev/null
+++ b/clang/test/OpenMP/metadirective_otherwise.cpp
@@ -0,0 +1,72 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-linux -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+void func1() {
+#pragma omp metadirective when(user = {condition(0)}	\
+			       : parallel for) otherwise()
+  for (int i = 0; i < 100; i++)
+    ;
+
+#pragma omp metadirective when(user = {condition(0)}	\
+			       : parallel for)
+  for (int i = 0; i < 100; i++)
+    ;
+}
+
+// CHECK-LABEL: define dso_local void @_Z5func1v()
+// CHECK:       entry
+// CHECK-NEXT:    [[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    [[I1:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i32 0, ptr [[I]], align 4
+// CHECK-NEXT:    br label %[[FOR_COND:.*]]
+// CHECK:       [[FOR_COND]]:
+// CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[I]], align 4
+// CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[TMP0]], 100
+// CHECK-NEXT:    br i1 [[CMP]], label %[[FOR_BODY:.*]], label %[[FOR_END:.*]]
+// CHECK:       [[FOR_BODY]]:
+// CHECK-NEXT:    br label %[[FOR_INC:.*]]
+// CHECK:       [[FOR_INC]]:
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr [[I]], align 4
+// CHECK-NEXT:    [[INC:%.*]] = add nsw i32 [[TMP1]], 1
+// CHECK-NEXT:    store i32 [[INC]], ptr [[I]], align 4
+// CHECK-NEXT:    br label %[[FOR_COND]], !llvm.loop [[LOOP3:![0-9]+]]
+// CHECK:       [[FOR_END]]:
+// CHECK-NEXT:    store i32 0, ptr [[I1]], align 4
+// CHECK-NEXT:    br label %[[FOR_COND2:.*]]
+// CHECK:       [[FOR_COND2]]:
+// CHECK-NEXT:    [[TMP2:%.*]] = load i32, ptr [[I1]], align 4
+// CHECK-NEXT:    [[CMP3:%.*]] = icmp slt i32 [[TMP2]], 100
+// CHECK-NEXT:    br i1 [[CMP3]], label %[[FOR_BODY4:.*]], label %[[FOR_END7:.*]]
+// CHECK:       [[FOR_BODY4]]:
+// CHECK-NEXT:    br label %[[FOR_INC5:.*]]
+// CHECK:       [[FOR_INC5]]:
+// CHECK-NEXT:    [[TMP3:%.*]] = load i32, ptr [[I1]], align 4
+// CHECK-NEXT:    [[INC6:%.*]] = add nsw i32 [[TMP3]], 1
+// CHECK-NEXT:    store i32 [[INC6]], ptr [[I1]], align 4
+// CHECK-NEXT:    br label %[[FOR_COND2]], !llvm.loop [[LOOP5:![0-9]+]]
+// CHECK:       [[FOR_END7]]:
+// CHECK-NEXT:    ret void
+
+void func2() {
+#pragma omp metadirective when(user = {condition(1)}	\
+			       : parallel for) otherwise()
+  for (int i = 0; i < 100; i++)
+    ;
+
+#pragma omp metadirective when(user = {condition(1)}	\
+			       : parallel for)
+  for (int i = 0; i < 100; i++)
+    ;
+}
+
+// CHECK-LABEL: define dso_local void @_Z5func2v()
+// CHECK:       entry
+// CHECK-NEXT:    call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @[[GLOB2:[0-9]+]], i32 0, ptr @_Z5func2v.omp_outlined)
+// CHECK-NEXT:    call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @[[GLOB2]], i32 0, ptr @_Z5func2v.omp_outlined.1)
+// CHECK-NEXT:    ret void
+
+
+#endif
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPContext.h b/llvm/include/llvm/Frontend/OpenMP/OMPContext.h
index a501eaf2356ff..26163fdb4b63d 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPContext.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPContext.h
@@ -188,7 +188,7 @@ bool isVariantApplicableInContext(const VariantMatchInfo &VMI,
                                   bool DeviceSetOnly = false);
 
 /// Return the index (into \p VMIs) of the variant with the highest score
-/// from the ones applicble in \p Ctx. See llvm::isVariantApplicableInContext.
+/// from the ones applicable in \p Ctx. See llvm::isVariantApplicableInContext.
 int getBestVariantMatchForContext(const SmallVectorImpl<VariantMatchInfo> &VMIs,
                                   const OMPContext &Ctx);

alexey-bataev · 2025-02-17T13:04:31Z

clang/lib/Parse/ParseOpenMP.cpp

+    if (BestIdx == -1 && Idx == 1) {
+      SkipUntil(tok::annot_pragma_openmp_end);
+    }


Not sure skipUntil is good here, since you may miss some junk at the end of the directive

Is this enough to not miss the potential upcoming tokens?

github-actions · 2025-02-17T16:41:51Z

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

alexey-bataev · 2025-02-17T18:16:40Z

clang/lib/Parse/ParseOpenMP.cpp

+    // OMP5.2 Chapter 7.4: If no otherwise clause is specified the effect is as
+    // if one was specified without an associated directive variant.
+    if (BestIdx == -1 && Idx == 1) {
+      SkipUntil(tok::annot_pragma_openmp_end,tok::identifier);


You still losing some data, SkipUntil does not work here

alexey-bataev · 2025-02-27T16:59:38Z

clang/lib/Parse/ParseOpenMP.cpp

+    if (BestIdx == -1 && Idx == 1) {
+      ConsumeAnnotationToken();
+      return StmtEmpty();
+    }


Do you have a test for the situation where you have extra tokens at this point? Or only tok::annot_pragma_openmp_end is expected here?

Line #89 of OpenMP/metadirective_ast_print.c show this situation.

Could you add an assertion here, that the next expected token is tok::annot_pragma_openmp_end?

The next expected token should be kw_for no?

I mean, before ConsumeAnnotationToken();

In all cases I am seeing, at this point Tok=annot_pragma_openmp_end and NextToken = kw_for. Asserting here would not work! But doesn't it make sense to have the kw_for right at the end of the pragma?

Yes, check that Tok=annot_pragma_openmp_end, this is what I meant

zahiraam · 2025-02-27T19:57:07Z

@alexey-bataev Thanks for the reviews.

…7113) Compiling this: `int main() {` ` #pragma omp metadirective when(use r= {condition(0)}` `: parallel for)` `for (int i=0; i<10; i++)` ; }` is generating an error: `error: expected expression` The compiler is interpreting this as if it's compiling a `#pragma omp metadirective` with no `otherwise` clause. In the OMP5.2 specs chapter 7.4 it's mentioned that: `If no otherwise clause is specified the effect is as if one was specified without an associated directive variant.` This patch fixes the issue.

zahiraam added 2 commits February 13, 2025 11:24

[OpenMP] Missing implicit otherwise clause in metadirective.

1995b54

Fix LIT test.

7fa46c6

zahiraam requested a review from alexey-bataev February 14, 2025 12:30

zahiraam marked this pull request as ready for review February 14, 2025 12:30

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

alexey-bataev reviewed Feb 17, 2025

View reviewed changes

Addressed review comments.

ab69f1e

alexey-bataev reviewed Feb 17, 2025

View reviewed changes

Addressed review comment.

c5160c0

alexey-bataev reviewed Feb 27, 2025

View reviewed changes

Added an assert.

9b4b94a

alexey-bataev approved these changes Feb 27, 2025

View reviewed changes

zahiraam merged commit 26fc3aa into llvm:main Feb 28, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OpenMP] Missing implicit otherwise clause in metadirective. #127113

[OpenMP] Missing implicit otherwise clause in metadirective. #127113

zahiraam commented Feb 13, 2025 •

edited

Loading

llvmbot commented Feb 14, 2025 •

edited

Loading

alexey-bataev Feb 17, 2025

zahiraam Feb 17, 2025 •

edited

Loading

github-actions bot commented Feb 17, 2025 •

edited

Loading

alexey-bataev Feb 17, 2025

alexey-bataev Feb 27, 2025

zahiraam Feb 27, 2025

alexey-bataev Feb 27, 2025

zahiraam Feb 27, 2025

alexey-bataev Feb 27, 2025

zahiraam Feb 27, 2025

alexey-bataev Feb 27, 2025

zahiraam commented Feb 27, 2025

[OpenMP] Missing implicit otherwise clause in metadirective. #127113

[OpenMP] Missing implicit otherwise clause in metadirective. #127113

Conversation

zahiraam commented Feb 13, 2025 • edited Loading

llvmbot commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

zahiraam Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2025 • edited Loading

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

zahiraam commented Feb 27, 2025

zahiraam commented Feb 13, 2025 •

edited

Loading

llvmbot commented Feb 14, 2025 •

edited

Loading

zahiraam Feb 17, 2025 •

edited

Loading

github-actions bot commented Feb 17, 2025 •

edited

Loading