[flang][OpenMP] Added parser support for in_reduction clause on OpenMP Task directive

[NimishMishra]

Summary

OpenMP 5.0 adds a new clause in_reduction on OpenMP task directive. This patch adds parser support for the same.

[kiranktp]

Thanks for the patch @NimishMishra
LGTM. Please wait for others to review and approve.

[shraiysh]

Please add a parse-tree test for this, like this one - https://github.com/llvm/llvm-project/blob/main/flang/test/Parser/omp-sections.f90

[shraiysh]

Why is the file name "omp-block-construct-unparse.f90"? We want to test in_reduction clause, why not add an omp task pragma unparsing test (if not already there). The name of the testfile is a little misleading. Same thing for the parse-tree tests. Rest LGTM.

[NimishMishra]

In D124156#3473639, @shraiysh wrote:

Why is the file name "omp-block-construct-unparse.f90"? We want to test in_reduction clause, why not add an omp task pragma unparsing test (if not already there). The name of the testfile is a little misleading. Same thing for the parse-tree tests. Rest LGTM.

I actually checked the current files in https://github.com/llvm/llvm-project/tree/main/flang/test/Parser and wondered if I should include one file that would contain all tests related to block constructs that we include in future. Many block construct related directives are absent in the flang/test/Parser directory; but I wondered we can condense any block directive incoming in future into this file. Let me know what you think about this.

[peixin]

Does this need changes in flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp? @kiranchandramohan

flang/include/flang/Parser/parse-tree.h

3437

[shraiysh]

I actually checked the current files in https://github.com/llvm/llvm-project/tree/main/flang/test/Parser and wondered if I should include one file that would contain all tests related to block constructs that we include in future. Many block construct related directives are absent in the flang/test/Parser directory; but I wondered we can condense any block directive incoming in future into this file. Let me know what you think about this.

I am assuming you are talking about OpenMP Block constructs and not there is no block construct in core fortran. In that case, their tests should be separated IMO. "omp-parallel-unparse.f90" should be separated from "omp-task-unparse.f90" as they are easily distinguishable. Nevertheless, I don't have a very strong opinion on this and am okay with "omp-block-construct-unparse.f90" too, but please add parse tree tests.

[NimishMishra]

Addressed comments

[shraiysh]

Minor comments. Thank you!

flang/test/Parser/omp-task-unparse.f90

5 ↗

(On Diff #430573)

Are you sure this program is semantically correct? I think an in_reduction clause must be enclosed inside task_reduction or some form of reduction clause. Having it at the top level could be an error, but I could be wrong. If it is not an error this is okay, otherwise a simple solution would be to change this to a subroutine instead of a program.

7 ↗

(On Diff #430573)

Please add some code in the structured block, maybe an x = x+1 or something actually reducing on x. Also just a suggestion, maybe check if it is generated while unparsing (to make sure we walk the children of such a node).

flang/test/Semantics/omp-clause-validity01.f90

66 ↗

(On Diff #430573)

Why are we adding semantics test when we have no semantic checks for this clause yet? IMO we should not be adding this test right now.

Anyways, if you decide to add this test, maybe add it as a separate test because private(b) and in_reduction(+:b) are probably conflicting. Also, please add some reducing code inside the structured block.

[peixin]

In D124156#3473770, @peixin wrote:

Does this need changes in flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp? @kiranchandramohan

LGTM. Please wait for @kiranchandramohan 's comments about this.

[kiranchandramohan]

In D124156#3527039, @peixin wrote:

In D124156#3473770, @peixin wrote:

Does this need changes in flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp? @kiranchandramohan

LGTM. Please wait for @kiranchandramohan 's comments about this.

It reports something like the following for the in_reduction clause which seems OK. You can build the tool with -DFLANG_BUILD_EXAMPLES=On and check with check-flang-examples. Run the tool like

$BUILD_DIR/bin/flang-new -fc1 -load $BUILD_DIR/lib/flangOmpReport.so -plugin flang-omp-report -fopenmp $FILE_NAME -o -

construct:       task
clauses:
  - clause:          in_reduction
    details:         '+:z'

flang/test/Semantics/omp-clause-validity01.f90

66 ↗

(On Diff #430573)

Possibly, it is checking the allowed clauses that are automatically created due to entries in OMP.td.

Not clear whether it is allowed or not. But something simple and separate would be better.

!$omp taskgroup task_reduction(+:z)
!$omp task in_reduction(+:z)
z = z + 5
!$omp end task
!$omp end taskgroup

We might also need to ask the task modifier to the regular reduction clause sometime.

[NimishMishra]

flang/test/Parser/omp-task-unparse.f90

5 ↗

(On Diff #430573)

Could you check this updated test case?

flang/test/Semantics/omp-clause-validity01.f90

66 ↗

(On Diff #430573)

I think @shraiysh is right in mentioning that I shouldn't be touching the semantics side of things. Removed the test case.

[kiranchandramohan]

LGTM.

flang/include/flang/Parser/dump-parse-tree.h

return llvm::Twine(
         "llvm::omp::Clause = ", llvm::omp::getOpenMPClauseName(x))
         .str();
   }
   NODE(parser, OmpObject)
   NODE(parser, OmpObjectList)
   NODE(parser, OmpProcBindClause)
   NODE_ENUM(OmpProcBindClause, Type)
   NODE(parser, OmpReductionClause)
+  NODE(parser, OmpInReductionClause)
   NODE(parser, OmpReductionCombiner)
   NODE(OmpReductionCombiner, FunctionCombiner)
   NODE(parser, OmpReductionInitializerClause)
   NODE(parser, OmpReductionOperator)
   NODE(parser, OmpAllocateClause)
   NODE(OmpAllocateClause, Allocator)
   NODE(parser, OmpScheduleClause)
   NODE_ENUM(OmpScheduleClause, ScheduleType)

flang/include/flang/Parser/parse-tree.h

 
 // 2.15.3.6 reduction-clause -> REDUCTION (reduction-identifier:
 //                                         variable-name-list)
 struct OmpReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpReductionClause);
   std::tuple<OmpReductionOperator, OmpObjectList> t;
 };
 
+// OMP 5.0 2.19.5.6 in_reduction-clause -> IN_REDUCTION (reduction-identifier:
+//                                         variable-name-list)
+struct OmpInReductionClause {
+  TUPLE_CLASS_BOILERPLATE(OmpInReductionClause);
+  std::tuple<OmpReductionOperator, OmpObjectList> t;
+};
+
 // OMP 5.0 2.11.4 allocate-clause -> ALLOCATE ([allocator:] variable-name-list)
 struct OmpAllocateClause {
   TUPLE_CLASS_BOILERPLATE(OmpAllocateClause);
   WRAPPER_CLASS(Allocator, ScalarIntExpr);
   std::tuple<std::optional<Allocator>, OmpObjectList> t;
 };
 
 // 2.13.9 depend-vec-length -> +/- non-negative-constant

flang/lib/Parser/openmp-parsers.cpp

 
 // 2.15.3.6 REDUCTION (reduction-identifier: variable-name-list)
 TYPE_PARSER(construct<OmpReductionOperator>(Parser<DefinedOperator>{}) ||
     construct<OmpReductionOperator>(Parser<ProcedureDesignator>{}))
 
 TYPE_PARSER(construct<OmpReductionClause>(
     Parser<OmpReductionOperator>{} / ":", Parser<OmpObjectList>{}))
 
+// OMP 5.0 2.19.5.6 IN_REDUCTION (reduction-identifier: variable-name-list)
+TYPE_PARSER(construct<OmpInReductionClause>(
+    Parser<OmpReductionOperator>{} / ":", Parser<OmpObjectList>{}))
+
 // OMP 5.0 2.11.4  ALLOCATE ([allocator:] variable-name-list)
 TYPE_PARSER(construct<OmpAllocateClause>(
     maybe(construct<OmpAllocateClause::Allocator>(scalarIntExpr) / ":"),
     Parser<OmpObjectList>{}))
 
 // 2.13.9 DEPEND (SOURCE | SINK : vec | (IN | OUT | INOUT) : list
 TYPE_PARSER(construct<OmpDependSinkVecLength>(
     Parser<DefinedOperator>{}, scalarIntConstantExpr))
TYPE_PARSER(
     "PRIORITY" >> construct<OmpClause>(construct<OmpClause::Priority>(
                       parenthesized(scalarIntExpr))) ||
     "PRIVATE" >> construct<OmpClause>(construct<OmpClause::Private>(
                      parenthesized(Parser<OmpObjectList>{}))) ||
     "PROC_BIND" >> construct<OmpClause>(construct<OmpClause::ProcBind>(
                        parenthesized(Parser<OmpProcBindClause>{}))) ||
     "REDUCTION" >> construct<OmpClause>(construct<OmpClause::Reduction>(
                        parenthesized(Parser<OmpReductionClause>{}))) ||
+    "IN_REDUCTION" >> construct<OmpClause>(construct<OmpClause::InReduction>(
+                          parenthesized(Parser<OmpInReductionClause>{}))) ||
     "TASK_REDUCTION" >>
         construct<OmpClause>(construct<OmpClause::TaskReduction>(
             parenthesized(Parser<OmpReductionClause>{}))) ||
     "RELAXED" >> construct<OmpClause>(construct<OmpClause::Relaxed>()) ||
     "RELEASE" >> construct<OmpClause>(construct<OmpClause::Release>()) ||
     "SAFELEN" >> construct<OmpClause>(construct<OmpClause::Safelen>(
                      parenthesized(scalarIntConstantExpr))) ||
     "SCHEDULE" >> construct<OmpClause>(construct<OmpClause::Schedule>(

flang/lib/Parser/unparse.cpp

void Unparse(const OmpLinearClause::WithModifier &x) {
     Walk(x.modifier), Put("("), Walk(x.names, ","), Put(")");
     Walk(":", x.step);
   }
   void Unparse(const OmpReductionClause &x) {
     Walk(std::get<OmpReductionOperator>(x.t));
     Put(":");
     Walk(std::get<OmpObjectList>(x.t));
   }
+  void Unparse(const OmpInReductionClause &x) {
+    Walk(std::get<OmpReductionOperator>(x.t));
+    Put(":");
+    Walk(std::get<OmpObjectList>(x.t));
+  }
   void Unparse(const OmpAllocateClause &x) {
     Walk(std::get<std::optional<OmpAllocateClause::Allocator>>(x.t));
     Put(":");
     Walk(std::get<OmpObjectList>(x.t));
   }
   void Unparse(const OmpDependSinkVecLength &x) {
     Walk(std::get<DefinedOperator>(x.t));
     Walk(std::get<ScalarIntConstantExpr>(x.t));

flang/test/Examples/omp-in-reduction-clause.f90

+! REQUIRES: plugins, examples, shell
+
+! RUN: %flang_fc1 -load %llvmshlibdir/flangOmpReport.so -plugin flang-omp-report -fopenmp  %s -o - | FileCheck %s
+
+! Check for IN_REDUCTION() clause on OpenMP constructs
+
+subroutine omp_in_reduction_taskgroup()
+    integer :: z, i
+    !$omp taskgroup task_reduction(+:z)
+    !$omp task in_reduction(+:z)
+        z = z + 5
+    !$omp end task
+
+    !$omp taskloop in_reduction(+:z)
+        do i=1,10
+            z = z * 5
+        end do
+    !$omp end taskloop 
+    !$omp end taskgroup
+end subroutine omp_in_reduction_taskgroup
+
+!CHECK: - file:         {{.*}}
+!CHECK:   line:         10
+!CHECK:   construct:    task
+!CHECK:   clauses:
+!CHECK:     - clause:   in_reduction
+!CHECK:       details:  '+:z'
+!CHECK: - file:         {{.*}}
+!CHECK:   line:         14
+!CHECK:   construct:    taskloop
+!CHECK:   clauses:
+!CHECK:     - clause:   in_reduction
+!CHECK:       details:  '+:z'
+!CHECK: - file:         {{.*}}
+!CHECK:   line:         9
+!CHECK:   construct:    taskgroup
+!CHECK:   clauses:
+!CHECK:      - clause:  task_reduction
+!CHECK:        details: '+:z'
+
+subroutine omp_in_reduction_parallel()
+    integer :: z
+    !$omp parallel reduction(+:z)
+        !$omp taskloop simd in_reduction(+:z)
+            do i=1,10
+                z = z * 5
+            end do
+        !$omp end taskloop simd
+    !$omp end parallel
+end subroutine omp_in_reduction_parallel
+
+!CHECK: - file:         {{.*}}
+!CHECK:   line:         44
+!CHECK:   construct:    taskloop simd
+!CHECK:   clauses:
+!CHECK:     - clause:   in_reduction
+!CHECK:       details:  '+:z'
+!CHECK:  - file:        {{.*}}
+!CHECK:    line:        43
+!CHECK:    construct:   parallel
+!CHECK:    clauses:
+!CHECK:      - clause:  reduction
+!CHECK:        details: '+:z'

flang/test/Parser/omp-in-reduction-clause.f90

+! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
+
+! Check for IN_REDUCTION() clause on OpenMP constructs
+
+subroutine omp_in_reduction_taskgroup()
+    integer :: z, i
+    !CHECK: !$OMP TASKGROUP  TASK_REDUCTION(+:z)
+    !$omp taskgroup task_reduction(+:z)
+    !CHECK-NEXT: !$OMP TASK  IN_REDUCTION(+:z)
+        !$omp task in_reduction(+:z)
+    !CHECK-NEXT: z=z+5_4
+            z = z + 5
+    !CHECK-NEXT: !$OMP END TASK
+        !$omp end task
+
+    !CHECK-NEXT: !$OMP TASKLOOP  IN_REDUCTION(+:z)
+        !$omp taskloop in_reduction(+:z)
+    !CHECK-NEXT: DO i=1_4,10_4
+            do i=1,10
+    !CHECK-NEXT: z=5_4*z
+                z = z * 5
+    !CHECK-NEXT: END DO
+            end do
+    !CHECK-NEXT: !$OMP END TASKLOOP
+        !$omp end taskloop
+    !CHECK-NEXT: !$OMP END TASKGROUP
+    !$omp end taskgroup
+end subroutine omp_in_reduction_taskgroup
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPBlockConstruct
+!PARSE-TREE-NEXT: OmpBeginBlockDirective
+!PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = taskgroup
+!PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> TaskReduction -> OmpReductionClause
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPBlockConstruct
+!PARSE-TREE-NEXT: OmpBeginBlockDirective
+!PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = task
+!PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> InReduction -> OmpInReductionClause
+!PARSE-TREE-NEXT: OmpReductionOperator -> DefinedOperator -> IntrinsicOperator = Add
+!PARSE-TREE-NEXT: OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'z'
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPLoopConstruct
+!PARSE-TREE-NEXT: OmpBeginLoopDirective
+!PARSE-TREE-NEXT: OmpLoopDirective -> llvm::omp::Directive = taskloop
+!PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> InReduction -> OmpInReductionClause
+!PARSE-TREE-NEXT: OmpReductionOperator -> DefinedOperator -> IntrinsicOperator = Add
+!PARSE-TREE-NEXT: OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'z'
+
+subroutine omp_in_reduction_parallel()
+    integer :: z
+    !CHECK: !$OMP PARALLEL  REDUCTION(+:z)
+    !$omp parallel reduction(+:z)
+    !CHECK-NEXT: !$OMP TASKLOOP SIMD  IN_REDUCTION(+:z)
+        !$omp taskloop simd in_reduction(+:z)
+    !CHECK-NEXT: DO i=1_4,10_4
+            do i=1,10
+    !CHECK-NEXT: z=5_4*z
+                z = z * 5
+    !CHECK-NEXT: END DO
+            end do
+    !CHECK-NEXT: !$OMP END TASKLOOP SIMD
+        !$omp end taskloop simd
+    !CHECK-NEXT: !$OMP END PARALLEL
+    !$omp end parallel
+end subroutine omp_in_reduction_parallel
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPBlockConstruct
+!PARSE-TREE-NEXT: OmpBeginBlockDirective
+!PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = parallel
+!PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> Reduction -> OmpReductionClause
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPLoopConstruct
+!PARSE-TREE-NEXT: OmpBeginLoopDirective
+!PARSE-TREE-NEXT: OmpLoopDirective -> llvm::omp::Directive = taskloop simd
+!PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> InReduction -> OmpInReductionClause
+!PARSE-TREE-NEXT: OmpReductionOperator -> DefinedOperator -> IntrinsicOperator = Add
+!PASRE-TREE-NEXT: OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'z'
+

llvm/include/llvm/Frontend/OpenMP/OMP.td

def OMPC_HasDeviceAddr : Clause<"has_device_addr"> {
   let isValueList = true;
 }
 def OMPC_TaskReduction : Clause<"task_reduction"> {
   let clangClass = "OMPTaskReductionClause";
   let flangClass = "OmpReductionClause";
 }
 def OMPC_InReduction : Clause<"in_reduction"> {
   let clangClass = "OMPInReductionClause";
+  let flangClass = "OmpInReductionClause";
 }
 def OMPC_UnifiedAddress : Clause<"unified_address"> {
   let clangClass = "OMPUnifiedAddressClause";
 }
 def OMPC_UnifiedSharedMemory : Clause<"unified_shared_memory"> {
   let clangClass = "OMPUnifiedSharedMemoryClause";
 }
 def OMPC_ReverseOffload : Clause<"reverse_offload"> {