[flang][OpenMP] Diagnose invalid reduction modifiers #92406

kparzysz · 2024-05-16T14:43:18Z

Emit diagnostic messages for invalid modifiers in "reduction" clause.

Fixes #92397

Emit diagnostic messages for invalid modifiers in "reduction" clause. Fixes llvm#92397

llvmbot · 2024-05-16T14:43:49Z

@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-semantics

Author: Krzysztof Parzyszek (kparzysz)

Changes

Emit diagnostic messages for invalid modifiers in "reduction" clause.

Fixes #92397

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

4 Files Affected:

(modified) flang/lib/Semantics/check-omp-structure.cpp (+50)
(modified) flang/lib/Semantics/check-omp-structure.h (+1)
(modified) flang/test/Lower/OpenMP/invalid-reduction-modifier.f90 (+1-3)
(added) flang/test/Semantics/OpenMP/reduction-modifiers.f90 (+91)

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 2493eb3ed3676..4377f093d062d 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2309,6 +2309,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
   if (CheckReductionOperators(x)) {
     CheckReductionTypeList(x);
   }
+  CheckReductionModifier(x);
 }
 
 bool OmpStructureChecker::CheckReductionOperators(
@@ -2393,6 +2394,55 @@ void OmpStructureChecker::CheckReductionTypeList(
   }
 }
 
+void OmpStructureChecker::CheckReductionModifier(
+    const parser::OmpClause::Reduction &x) {
+  using ReductionModifier = parser::OmpReductionClause::ReductionModifier;
+  const auto &maybeModifier{std::get<std::optional<ReductionModifier>>(x.v.t)};
+  if (!maybeModifier || *maybeModifier == ReductionModifier::Default) {
+    // No modifier, or the default one is always ok.
+    return;
+  }
+  ReductionModifier modifier{*maybeModifier};
+  const DirectiveContext &dirCtx{GetContext()};
+  if (modifier == ReductionModifier::Task) {
+    // "Task" is only allowed on worksharing or "parallel" directive.
+    static llvm::omp::Directive worksharing[]{
+        llvm::omp::Directive::OMPD_do,
+        // llvm::omp::Directive::OMPD_for, C++ only
+        llvm::omp::Directive::OMPD_loop,
+        llvm::omp::Directive::OMPD_scope,
+        llvm::omp::Directive::OMPD_sections,
+        llvm::omp::Directive::OMPD_single,
+        llvm::omp::Directive::OMPD_workshare,
+    };
+    if (dirCtx.directive != llvm::omp::Directive::OMPD_parallel &&
+        !llvm::is_contained(worksharing, dirCtx.directive)) {
+      context_.Say(GetContext().clauseSource,
+                   "Modifier 'TASK' on REDUCTION clause is only allowed with "
+                   "PARALLEL or worksharing directive"_err_en_US);
+    }
+  } else if (modifier == ReductionModifier::Inscan) {
+    // Inscan is only allowed on worksharing-loop, worksharing-loop simd,
+    // or "simd" directive.
+    // The worksharing-loop directives are OMPD_do and OMPD_for. Only the
+    // former is allowed in Fortran.
+    switch (dirCtx.directive) {
+    case llvm::omp::Directive::OMPD_do:      // worksharing-loop
+    case llvm::omp::Directive::OMPD_do_simd: // worksharing-loop simd
+    case llvm::omp::Directive::OMPD_simd:    // "simd"
+      break;
+    default:
+      context_.Say(GetContext().clauseSource,
+                   "Modifier 'INSCAN' on REDUCTION clause is only allowed with "
+                   "worksharing-loop, worksharing-loop simd, "
+                   "or SIMD directive"_err_en_US);
+    }
+  } else {
+    context_.Say(GetContext().clauseSource, "Unexpected modifier on REDUCTION "
+                                            "clause"_err_en_US);
+  }
+}
+
 void OmpStructureChecker::CheckIntentInPointerAndDefinable(
     const parser::OmpObjectList &objectList, const llvm::omp::Clause clause) {
   for (const auto &ompObject : objectList.v) {
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 1f7284307703b..47705771e8e28 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -205,6 +205,7 @@ class OmpStructureChecker
   bool CheckIntrinsicOperator(
       const parser::DefinedOperator::IntrinsicOperator &);
   void CheckReductionTypeList(const parser::OmpClause::Reduction &);
+  void CheckReductionModifier(const parser::OmpClause::Reduction &);
   void CheckMasterNesting(const parser::OpenMPBlockConstruct &x);
   void ChecksOnOrderedAsBlock();
   void CheckBarrierNesting(const parser::OpenMPSimpleStandaloneConstruct &x);
diff --git a/flang/test/Lower/OpenMP/invalid-reduction-modifier.f90 b/flang/test/Lower/OpenMP/invalid-reduction-modifier.f90
index 53871276761fa..b3e87df7086eb 100644
--- a/flang/test/Lower/OpenMP/invalid-reduction-modifier.f90
+++ b/flang/test/Lower/OpenMP/invalid-reduction-modifier.f90
@@ -1,6 +1,4 @@
-!Remove the --crash below once we can diagnose the issue more gracefully.
-!REQUIRES: asserts
-!RUN: not --crash %flang_fc1 -fopenmp -emit-hlfir -o - %s
+!RUN: not %flang_fc1 -fopenmp -emit-hlfir -o - %s
 
 ! Check that we reject the "task" reduction modifier on the "simd" directive.
 
diff --git a/flang/test/Semantics/OpenMP/reduction-modifiers.f90 b/flang/test/Semantics/OpenMP/reduction-modifiers.f90
new file mode 100644
index 0000000000000..3e6a856f60310
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/reduction-modifiers.f90
@@ -0,0 +1,91 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp -fopenmp-version=52
+
+subroutine mod_task1(x)
+  integer, intent(inout) :: x
+
+  !Correct: "parallel" directive.
+  !$omp parallel reduction(task, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end parallel
+end
+
+subroutine mod_task2(x)
+  integer, intent(inout) :: x
+
+  !Correct: worksharing directive.
+  !$omp sections reduction(task, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end sections
+end
+
+
+subroutine mod_task3(x)
+  integer, intent(inout) :: x
+
+  !ERROR: Modifier 'TASK' on REDUCTION clause is only allowed with PARALLEL or worksharing directive
+  !$omp simd reduction(task, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end simd
+end
+
+subroutine mod_inscan1(x)
+  integer, intent(inout) :: x
+
+  !Correct: worksharing-loop directive
+  !$omp do reduction(inscan, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end do
+end
+
+subroutine mod_inscan2(x)
+  integer, intent(inout) :: x
+
+  !Correct: worksharing-loop simd directive
+  !$omp do simd reduction(inscan, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end do simd
+end
+
+subroutine mod_inscan3(x)
+  integer, intent(inout) :: x
+
+  !Correct: "simd" directive
+  !$omp simd reduction(inscan, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end simd
+end
+
+subroutine mod_inscan4(x)
+  integer, intent(inout) :: x
+
+  !ERROR: Modifier 'INSCAN' on REDUCTION clause is only allowed with worksharing-loop, worksharing-loop simd, or SIMD directive
+  !$omp parallel reduction(inscan, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end parallel
+end
+
+subroutine mod_inscan5(x)
+  integer, intent(inout) :: x
+
+  !ERROR: Modifier 'INSCAN' on REDUCTION clause is only allowed with worksharing-loop, worksharing-loop simd, or SIMD directive
+  !$omp sections reduction(inscan, +:x)
+  do i = 1, 100
+    x = foo(i)
+  enddo
+  !$omp end sections
+end
+

github-actions · 2024-05-16T14:46:29Z

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

kiranchandramohan

Thanks for the patch Kryztof.

There are some additional mentions for restrictions of combined/composite construct.

kiranchandramohan · 2024-05-18T13:30:56Z

flang/lib/Semantics/check-omp-structure.cpp

+    }
+  } else {
+    // Catch-all for potential future modifiers to make sure that this
+    // function is up-tp-date.


Suggested change

// function is up-tp-date.

// function is up-to-date.

kiranchandramohan · 2024-05-18T13:40:38Z

flang/lib/Semantics/check-omp-structure.cpp

+    static llvm::omp::Directive worksharing[]{
+        llvm::omp::Directive::OMPD_do,
+        // llvm::omp::Directive::OMPD_for, C++ only
+        llvm::omp::Directive::OMPD_loop,


Is loop allowed?

In the restrictions for loop directive. Page 257 OMP 5.2
If a reduction-modifier is specified in a reduction clause that appears on the directive then the reduction modifier must be default.

Added a separate check for "loop".

kiranchandramohan · 2024-05-18T13:48:22Z

flang/lib/Semantics/check-omp-structure.cpp

+        llvm::omp::Directive::OMPD_single,
+        llvm::omp::Directive::OMPD_workshare,


Reduction clause is not allowed for these two, but I guess it is alright to have it here.

Removed with a comment that they are not allowed with "reduction".

kparzysz · 2024-05-20T13:26:27Z

I couldn't add a testcase for "loop" because the parser doesn't recognize "loop" yet.

kiranchandramohan

LG.

tblah

LGTM, thanks!

[flang][OpenMP] Diagnose invalid reduction modifiers

2854e27

Emit diagnostic messages for invalid modifiers in "reduction" clause. Fixes llvm#92397

kparzysz requested review from tblah and psteinfeld May 16, 2024 14:43

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics labels May 16, 2024

Minor formatting changes

4dc8ed1

kparzysz mentioned this pull request May 16, 2024

[Frontend][OpenMP] Reduction modifier must be applied somewhere #92160

Merged

kparzysz added 2 commits May 16, 2024 09:52

clang-format

74ad5a2

More clang-format

903ffaf

kiranchandramohan reviewed May 18, 2024

View reviewed changes

Apply review comments

7eb0ef2

Reword comment, formatting

5e1efff

kiranchandramohan approved these changes May 22, 2024

View reviewed changes

tblah approved these changes May 22, 2024

View reviewed changes

kparzysz merged commit 2aa218c into llvm:main May 22, 2024
4 checks passed

kparzysz deleted the users/kparzysz/reduction-modifiers branch May 22, 2024 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][OpenMP] Diagnose invalid reduction modifiers #92406

[flang][OpenMP] Diagnose invalid reduction modifiers #92406

kparzysz commented May 16, 2024

llvmbot commented May 16, 2024 •

edited

github-actions bot commented May 16, 2024 •

edited

kiranchandramohan left a comment

kiranchandramohan May 18, 2024

kparzysz May 20, 2024

kiranchandramohan May 18, 2024

kiranchandramohan May 18, 2024

kparzysz May 20, 2024

kiranchandramohan May 18, 2024

kparzysz May 20, 2024

kparzysz commented May 20, 2024

kiranchandramohan left a comment

tblah left a comment

		llvm::omp::Directive::OMPD_single,
		llvm::omp::Directive::OMPD_workshare,

[flang][OpenMP] Diagnose invalid reduction modifiers #92406

[flang][OpenMP] Diagnose invalid reduction modifiers #92406

Conversation

kparzysz commented May 16, 2024

llvmbot commented May 16, 2024 • edited

github-actions bot commented May 16, 2024 • edited

kiranchandramohan left a comment

Choose a reason for hiding this comment

kiranchandramohan May 18, 2024

Choose a reason for hiding this comment

kparzysz May 20, 2024

Choose a reason for hiding this comment

kiranchandramohan May 18, 2024

Choose a reason for hiding this comment

kiranchandramohan May 18, 2024

Choose a reason for hiding this comment

kparzysz May 20, 2024

Choose a reason for hiding this comment

kiranchandramohan May 18, 2024

Choose a reason for hiding this comment

kparzysz May 20, 2024

Choose a reason for hiding this comment

kparzysz commented May 20, 2024

kiranchandramohan left a comment

Choose a reason for hiding this comment

tblah left a comment

Choose a reason for hiding this comment

llvmbot commented May 16, 2024 •

edited

github-actions bot commented May 16, 2024 •

edited