From 2854e272346baad7d5e9a12a8c8243a7c5b210b3 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 16 May 2024 09:40:51 -0500 Subject: [PATCH 1/6] [flang][OpenMP] Diagnose invalid reduction modifiers Emit diagnostic messages for invalid modifiers in "reduction" clause. Fixes https://github.com/llvm/llvm-project/issues/92397 --- flang/lib/Semantics/check-omp-structure.cpp | 50 ++++++++++ flang/lib/Semantics/check-omp-structure.h | 1 + .../OpenMP/invalid-reduction-modifier.f90 | 4 +- .../Semantics/OpenMP/reduction-modifiers.f90 | 91 +++++++++++++++++++ 4 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 flang/test/Semantics/OpenMP/reduction-modifiers.f90 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>(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 + From 4dc8ed103b60bff578cca8bc0b42eca5b9715175 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 16 May 2024 09:45:15 -0500 Subject: [PATCH 2/6] Minor formatting changes --- flang/lib/Semantics/check-omp-structure.cpp | 2 +- flang/test/Semantics/OpenMP/reduction-modifiers.f90 | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index 4377f093d062d..ba4e9b548412d 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -2422,7 +2422,7 @@ void OmpStructureChecker::CheckReductionModifier( "PARALLEL or worksharing directive"_err_en_US); } } else if (modifier == ReductionModifier::Inscan) { - // Inscan is only allowed on worksharing-loop, worksharing-loop simd, + // "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. diff --git a/flang/test/Semantics/OpenMP/reduction-modifiers.f90 b/flang/test/Semantics/OpenMP/reduction-modifiers.f90 index 3e6a856f60310..cf38200ba0a83 100644 --- a/flang/test/Semantics/OpenMP/reduction-modifiers.f90 +++ b/flang/test/Semantics/OpenMP/reduction-modifiers.f90 @@ -22,7 +22,6 @@ subroutine mod_task2(x) !$omp end sections end - subroutine mod_task3(x) integer, intent(inout) :: x @@ -88,4 +87,3 @@ subroutine mod_inscan5(x) enddo !$omp end sections end - From 74ad5a22883cecae65226c6d70a4dd34b15ace33 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 16 May 2024 09:52:27 -0500 Subject: [PATCH 3/6] clang-format --- flang/lib/Semantics/check-omp-structure.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index ba4e9b548412d..f20a06dd9f7d5 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -2418,8 +2418,8 @@ void OmpStructureChecker::CheckReductionModifier( 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); + "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, @@ -2427,19 +2427,21 @@ void OmpStructureChecker::CheckReductionModifier( // 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: // worksharing-loop case llvm::omp::Directive::OMPD_do_simd: // worksharing-loop simd - case llvm::omp::Directive::OMPD_simd: // "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); + "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); + // Catch-all for potential future modifiers to make sure that this + // function is up-tp-date. + context_.Say(GetContext().clauseSource, + "Unexpected modifier on REDUCTION clause"_err_en_US); } } From 903ffaf25625e63d1a35506b21bd5ce1de820184 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 16 May 2024 09:56:28 -0500 Subject: [PATCH 4/6] More clang-format --- flang/lib/Semantics/check-omp-structure.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index f20a06dd9f7d5..5c057c5149fc8 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -2441,7 +2441,7 @@ void OmpStructureChecker::CheckReductionModifier( // Catch-all for potential future modifiers to make sure that this // function is up-tp-date. context_.Say(GetContext().clauseSource, - "Unexpected modifier on REDUCTION clause"_err_en_US); + "Unexpected modifier on REDUCTION clause"_err_en_US); } } From 7eb0ef29fdb276c27dacf5fc54a12428a291ad3a Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Mon, 20 May 2024 08:24:28 -0500 Subject: [PATCH 5/6] Apply review comments --- flang/lib/Semantics/check-omp-structure.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index 5c057c5149fc8..5e55c380ffbe2 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -2404,16 +2404,24 @@ void OmpStructureChecker::CheckReductionModifier( } ReductionModifier modifier{*maybeModifier}; const DirectiveContext &dirCtx{GetContext()}; + if (dirCtx.directive == llvm::omp::Directive::OMPD_loop) { + // [5.2:257:33-34] + // If a reduction-modifier is specified in a reduction clause that + // appears on the directive, then the reduction modifier must be + // default. + context_.Say(GetContext().clauseSource, + "REDUCTION modifier on LOOP directive must be DEFAULT"_err_en_US); + } 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, + // "single" and "workshare" are worksharing directives, but + // they don't allow reduction clause. + // "loop" is also worksharing, but has different restrictions. }; if (dirCtx.directive != llvm::omp::Directive::OMPD_parallel && !llvm::is_contained(worksharing, dirCtx.directive)) { @@ -2439,7 +2447,7 @@ void OmpStructureChecker::CheckReductionModifier( } } else { // Catch-all for potential future modifiers to make sure that this - // function is up-tp-date. + // function is up-to-date. context_.Say(GetContext().clauseSource, "Unexpected modifier on REDUCTION clause"_err_en_US); } From 5e1efffd0657bf9cfda275bcd954c0e83ac6e97f Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Mon, 20 May 2024 08:30:28 -0500 Subject: [PATCH 6/6] Reword comment, formatting --- flang/lib/Semantics/check-omp-structure.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index 5e55c380ffbe2..a98c2618e3ff4 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -2415,13 +2415,12 @@ void OmpStructureChecker::CheckReductionModifier( 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_scope, + llvm::omp::Directive::OMPD_do, llvm::omp::Directive::OMPD_scope, llvm::omp::Directive::OMPD_sections, - // "single" and "workshare" are worksharing directives, but - // they don't allow reduction clause. - // "loop" is also worksharing, but has different restrictions. + // There are more worksharing directives, but they do not apply: + // "for" is C++ only, + // "single" and "workshare" don't allow reduction clause, + // "loop" has different restrictions (checked above). }; if (dirCtx.directive != llvm::omp::Directive::OMPD_parallel && !llvm::is_contained(worksharing, dirCtx.directive)) {