[Flang][openmp][5.0] Add task_reduction clause.

[sameeranjoshi]

Summary

See OMP-5.0 2.19.5.5 task_reduction Clause.
To add a positive test case we need taskgroup directive which is not added hence skipping the test.
This is a dependency for taskgroup construct.

Co-authored-by: Valentin Clement <[e-mail]>

[clementval]

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

3423

Is there a problem with the formatting here?

3426

Is there a good reason why OmpReductionClause takes a list of Designator and here you take an OmpObjectList?

flang/lib/Parser/unparse.cpp

2029

Extra blank line.

[sameeranjoshi]

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

3426

I did ask once the same in slack channel.
But I think the complicated checks can be done in semantic part given the tablegen framework.
https://flang-compiler.slack.com/archives/CGB8G85QV/p1603382608040500

[clementval]

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

3426

I think they should be the same and possibly use the same node for that.

[sameeranjoshi]

Update OmpReductionClause to use OmpObjectList.
Make OmpTaskReductionClause depend on OmpReductionClause.
This change was needed to create consistency among the reduction clauses.

[sameeranjoshi]

Thank you for reviewing.

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

3426

OmpReductionClause now uses OmpObjectList.

[clementval]

flang/lib/Parser/openmp-parsers.cpp

108

Do you really need this indirection? Why not using OmpReductionClause directly?

[clementval]

Remove indirection and use OmpReductionClause directly.

[clementval]

flang/lib/Parser/openmp-parsers.cpp

225

@sameeranjoshi After our slack discussion, I hope you don't mind but I updated your patch with a working version without the indirection.

The reduction clause is actually not done "right" at the moment since it doesn't follow other clauses. It should be done in a similar way as TASK_REDUCTION now. I guess we can do this in a separated patch.

flang/lib/Parser/unparse.cpp

2016

This will be changed in a follow up patch to reflect the change in TaskReduction and Reduction.

[clementval]

I guess it looks like what we have discussed now.

If you want to attribute correctly the co-authorship of this patch, you can add Co-authored-by: Valentin Clement <[e-mail]> at the end of the commit message. GitHub will do the rest.

[kiranchandramohan]

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

[clementval]

flang/lib/Parser/openmp-parsers.cpp

225

update to reduction clause are done in D93482

[yhegde]

In D93105#2461225, @kiranchandramohan wrote:

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

Will take a look and get back to you.

[yhegde]

In D93105#2461225, @kiranchandramohan wrote:

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

Yes. Rt now I am directly getting the list of Designators . Now it will become a level of indirection with the list of OmpObjects.
I may be missing something but OpenMP 4.5 and 5.0 Reduction clause restrictions looks very similar , why this one level of indirection required?! Is it because functions names in Reduction clause can come from common blocks ?!

[sameeranjoshi]

In D93105#2464268, @yhegde wrote:

In D93105#2461225, @kiranchandramohan wrote:

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

Yes. Rt now I am directly getting the list of Designators . Now it will become a level of indirection with the list of OmpObjects.
I may be missing something but OpenMP 4.5 and 5.0 Reduction clause restrictions looks very similar , why this one level of indirection required?! Is it because functions names in Reduction clause can come from common blocks ?!

I am not sure if I completely understand your question.
struct OmpObject wraps over std::variant<Designator, /*common block*/ Name> u, and if you were trying to ask why did we change that from Designator to OmpObject check comments at https://reviews.llvm.org/D93105#inline-870822.

[clementval]

The co-authorship must be in the commit message. Updating phab doesn't change what was landed. Look at https://github.com/llvm/llvm-project/commit/8a58f21f5b6c228137a9b87906fe5b720c4d1dfb, the co-authorship is correctly displayed on GitHub. This is not the case with the commit you landed for this revision.

[clementval]

Reopen the revision since it was reverted and other patches depend on it.

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

struct OmpReductionOperator {
   UNION_CLASS_BOILERPLATE(OmpReductionOperator);
   std::variant<DefinedOperator, ProcedureDesignator> u;
 };
 
 // 2.15.3.6 reduction-clause -> REDUCTION (reduction-identifier:
 //                                         variable-name-list)
 struct OmpReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpReductionClause);
-  std::tuple<OmpReductionOperator, std::list<Designator>> t;
+  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
 struct OmpDependSinkVecLength {
   TUPLE_CLASS_BOILERPLATE(OmpDependSinkVecLength);
   std::tuple<DefinedOperator, ScalarIntConstantExpr> t;
 };
 
 // 2.13.9 depend-vec -> iterator [+/- depend-vec-length],...,iterator[...]

flang/lib/Parser/openmp-parsers.cpp

maybe(
         ":"),
     scalarLogicalExpr))
 
 // 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>{} / ":", nonemptyList(designator)))
+    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(
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>(parenthesized(Parser<OmpReductionClause>{})) ||
+    "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>(parenthesized(Parser<OmpScheduleClause>{})) ||
     "SEQ_CST" >> construct<OmpClause>(construct<OmpClause::SeqCst>()) ||
     "SHARED" >> construct<OmpClause>(construct<OmpClause::Shared>(

flang/lib/Parser/unparse.cpp

void Unparse(const OmpLinearClause::WithoutModifier &x) {
     Put(")");
   }
   void Unparse(const OmpLinearClause::WithModifier &x) {
     Word("LINEAR("), Walk(x.modifier), Put("("), Walk(x.names, ","), Put(")");
     Walk(":", x.step);
     Put(")");
   }
   void Unparse(const OmpReductionClause &x) {
     Word("REDUCTION(");
     Walk(std::get<OmpReductionOperator>(x.t));
     Put(":");
-    Walk(std::get<std::list<Designator>>(x.t), ",");
+    Walk(std::get<OmpObjectList>(x.t));
     Put(")");
   }
   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));
   }
   void Unparse(const OmpDependSinkVec &x) {
     Walk(std::get<Name>(x.t));
     Walk(std::get<std::optional<OmpDependSinkVecLength>>(x.t));
   }
   void Unparse(const OmpDependClause::InOut &x) {
     Put("(");
     Walk(std::get<OmpDependenceType>(x.t));

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

#include "llvm/Frontend/OpenMP/OMP.inc"
   void Enter(const parser::OmpClause::NumThreads &);
   void Enter(const parser::OmpClause::Ordered &);
   void Enter(const parser::OmpClause::Priority &);
   void Enter(const parser::OmpClause::Private &);
   void Enter(const parser::OmpClause::ProcBind &);
   void Enter(const parser::OmpClause::Safelen &);
   void Enter(const parser::OmpClause::Shared &);
   void Enter(const parser::OmpClause::Simdlen &);
+  void Enter(const parser::OmpClause::TaskReduction &);
   void Enter(const parser::OmpClause::ThreadLimit &);
   void Enter(const parser::OmpClause::To &);
   void Enter(const parser::OmpClause::Link &);
   void Enter(const parser::OmpClause::Uniform &);
   void Enter(const parser::OmpClause::UseDevicePtr &);
   void Enter(const parser::OmpClause::IsDevicePtr &);
   // Memory-order-clause
   void Enter(const parser::OmpClause::SeqCst &);

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

 CHECK_SIMPLE_CLAUSE(Inbranch, OMPC_inbranch)
 CHECK_SIMPLE_CLAUSE(IsDevicePtr, OMPC_is_device_ptr)
 CHECK_SIMPLE_CLAUSE(Lastprivate, OMPC_lastprivate)
 CHECK_SIMPLE_CLAUSE(Link, OMPC_link)
 CHECK_SIMPLE_CLAUSE(Mergeable, OMPC_mergeable)
 CHECK_SIMPLE_CLAUSE(Nogroup, OMPC_nogroup)
 CHECK_SIMPLE_CLAUSE(Notinbranch, OMPC_notinbranch)
 CHECK_SIMPLE_CLAUSE(Nowait, OMPC_nowait)
+CHECK_SIMPLE_CLAUSE(TaskReduction, OMPC_task_reduction)
 CHECK_SIMPLE_CLAUSE(To, OMPC_to)
 CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform)
 CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied)
 CHECK_SIMPLE_CLAUSE(UseDevicePtr, OMPC_use_device_ptr)
 CHECK_SIMPLE_CLAUSE(AcqRel, OMPC_acq_rel)
 CHECK_SIMPLE_CLAUSE(Acquire, OMPC_acquire)
 CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst)
 CHECK_SIMPLE_CLAUSE(Release, OMPC_release)

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

 !                      linear-clause |
 !                      aligned-clause |
 !                      private-clause |
 !                      lastprivate-clause |
 !                      reduction-clause |
 !                      collapse-clause
 
   a = 0.0
-  !$omp simd private(b) reduction(+:a)
+  !ERROR: TASK_REDUCTION clause is not allowed on the SIMD directive
+  !$omp simd private(b) reduction(+:a) task_reduction(+:a)
   do i = 1, N
      a = a + b + 3.14
   enddo
 
   !ERROR: At most one SAFELEN clause can appear on the SIMD directive
   !$omp simd safelen(1) safelen(2)
   do i = 1, N
      a = 3.14
!                          nogroup-clause
 
   !ERROR: GRAINSIZE and NUM_TASKS clauses are mutually exclusive and may not appear on the same TASKLOOP directive
   !$omp taskloop num_tasks(3) grainsize(2)
   do i = 1,N
      a = 3.14
   enddo
 
   !ERROR: At most one NUM_TASKS clause can appear on the TASKLOOP directive
-  !$omp taskloop num_tasks(3) num_tasks(2)
+  !ERROR: TASK_REDUCTION clause is not allowed on the TASKLOOP directive
+  !$omp taskloop num_tasks(3) num_tasks(2) task_reduction(*:a)
   do i = 1,N
     a = 3.14
   enddo
 
 ! 2.13.1 master
 
   !$omp parallel
   !$omp master

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

 }
 def OMPC_IsDevicePtr : Clause<"is_device_ptr"> {
   let clangClass = "OMPIsDevicePtrClause";
   let flangClassValue = "Name";
   let isValueList = true;
 }
 def OMPC_TaskReduction : Clause<"task_reduction"> {
   let clangClass = "OMPTaskReductionClause";
+  let flangClassValue = "OmpReductionClause";
 }
 def OMPC_InReduction : Clause<"in_reduction"> {
   let clangClass = "OMPInReductionClause";
 }
 def OMPC_UnifiedAddress : Clause<"unified_address"> {
   let clangClass = "OMPUnifiedAddressClause";
 }
 def OMPC_UnifiedSharedMemory : Clause<"unified_shared_memory"> {