[Flang][Openmp] Upgrade TASKGROUP construct to 5.0.

[AMDChirag]

Summary

In OMP 5.0 specification clause-list with

• task_reduction
• allocate

were allowed on taskgroup construct.

Also updates some Clang test cases to use OpenMP 5.0

Fix XFAIL - omp-taskloop01.f90.

Patch created originally by @sameeranjoshi

[sameeranjoshi]

Fix newlines.

[clementval]

flang/lib/Lower/OpenMP.cpp

242

If you add the todo can you maybe explicitly add an error message in it?

flang/test/Semantics/omp-taskgroup01.f90

37

Why is this block commented out?

[sameeranjoshi]

flang/lib/Lower/OpenMP.cpp

242

What in case we remove it, that's ok is it?

flang/test/Semantics/omp-taskgroup01.f90

37

Depends on D93105

[clementval]

flang/lib/Lower/OpenMP.cpp

242

Fine as well. In the current state it doesn't add more.

flang/test/Semantics/omp-taskgroup01.f90

37

Yeah but since you have it as a dependency in phab you can uncomment the block.

[clementval]

flang/test/Semantics/omp-taskgroup01.f90

37

What I mean is that the pre-merge check for example will apply the patch you depend on before applying this one so you will not have failure.

[sameeranjoshi]

Address review comments.

[kiranchandramohan]

flang/test/Semantics/omp-taskgroup01.f90

40

A colon (not comma) is expected right?

[kiranchandramohan]

flang/test/Semantics/omp-taskgroup01.f90

38

reduction_var is not declared?

49

Should this be taskgroup or is this accidentally added?

52

Check count of begin and end taskgroups.

[sameeranjoshi]

Summary of changes:

• Simplify the test as those checks were already covered.
• Update OMP.td to specifiy to add 5.0 standard for the clauses.

Rebase after task_reduction(D93105).

[sameeranjoshi]

These are great catches, thank you.
I was wondering since the merge of task_reduction why were the tests failing and did kept this on hold for a while since then.
Thanks again. :)

flang/test/Semantics/omp-taskgroup01.f90

40

Ideally it was supposed to be caught in parser itself, unfortunately it didn't.
May be this needs to be investigated in new patch.

[sameeranjoshi]

Ping.

[kiranchandramohan]

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

443 ↗

(On Diff #316180)

Are tests failing due to task_reduction/allocate marked as version 5.0?

[sameeranjoshi]

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

443 ↗

(On Diff #316180)

All clang related OpenMP tests are failing which contain flag -fopenmp-version=45 for compilation along with -fopenmp.
The difference of 4.5 and 5.0 specifically mentions these clauses to in 5.0 standard which was the reason to add 50.
Should we remove this string do is there some other place to change in TableGen?

[kiranchandramohan]

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

443 ↗

(On Diff #316180)

I think in general for these situation we have chosen not to add the 5.0 version. Is that right @clementval?

But correcting these testcases is probably the right thing to do. But for that we might need some inputs from @jdoerfert or @Meinersbur.

[clementval]

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

443 ↗

(On Diff #316180)

Flang is not using this information now but clang is using it. So if you introduce the version on those clauses and some clang/openmp tests are failing they should be investigated.

[AMDChirag]

Fixes the clang test cases

[AMDChirag]

Rebased to main

[AMDChirag]

Hi @kiranchandramohan How do I restart the build? It says I don't have permission.

[kiranchandramohan]

It could be because you are not the author. Can you commandeer the patch (option available in the Add Action menu below)?

[AMDChirag]

In D93373#2832533, @kiranchandramohan wrote:

It could be because you are not the author. Can you commandeer the patch (option available in the Add Action menu below)?

Just did that. Still the same error:

You do not have permission to edit this object.
Users with the "Can Edit" capability:
Administrators can take this action.
You must have edit permission on this build plan to pause, abort, resume, or restart it.You must have edit permission on this build plan to run it manually.

[kiranchandramohan]

Can you rebase and update the patch? It seems to have built for the latest revision (Harbormaster completed remote builds in B110336: Diff 353548).

[AMDChirag]

Rebased

[AMDChirag]

Oh great! That triggered it!
Thank you!

[AMDChirag]

@kiranchandramohan It failed again. Apparently, it automatically attempted to add a patch D93105 which is already merged. I removed that patch as dependency before (edit related revisions), but it seems to have been added again.
Am I missing something?

https://buildkite.com/llvm-project/diff-checks/builds/49889#af4f049f-bb84-485c-8b30-9ca91ecc1bb9

[AMDChirag]

Rebased

[AMDChirag]

@kiranchandramohan The tests passed now!

[AMDChirag]

@jdoerfert Could you please take a look at this patch as well? (especially for the clang part)

[kiranchandramohan]

LGTM. Please update the commit message with information about changes in Clang tests as well.

[AMDChirag]

Rebase to current main branch

[AMDChirag]

Removed parent line in summary, it was not longer relevant and caused build failures.

[AMDChirag]

Minor test fix

[AMDChirag]

Rebased. Verified, this patch is not causing any test failures for me.

[AMDChirag]

Minor fix in test case; Rebased.

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

updated commit message

flang/lib/Lower/OpenMP.cpp

for (const auto &clause : parallelOpClauseList.v) {
         }
       }
     }
     createBodyOfOp<omp::ParallelOp>(parallelOp, firOpBuilder, currentLocation);
   } else if (blockDirective.v == llvm::omp::OMPD_master) {
     auto masterOp =
         firOpBuilder.create<mlir::omp::MasterOp>(currentLocation, argTy);
     createBodyOfOp<omp::MasterOp>(masterOp, firOpBuilder, currentLocation);
+  } else if (blockDirective.v == llvm::omp::OMPD_taskgroup) {
+    TODO("");
   }
 }
 
 void Fortran::lower::genOpenMPConstruct(
     Fortran::lower::AbstractConverter &converter,
     Fortran::lower::pft::Evaluation &eval,
     const Fortran::parser::OpenMPConstruct &ompConstruct) {
 

flang/lib/Semantics/resolve-directives.cpp

bool OmpAttributeVisitor::Pre(const parser::OpenMPBlockConstruct &x) {
   switch (beginDir.v) {
   case llvm::omp::Directive::OMPD_master:
   case llvm::omp::Directive::OMPD_ordered:
   case llvm::omp::Directive::OMPD_parallel:
   case llvm::omp::Directive::OMPD_single:
   case llvm::omp::Directive::OMPD_target:
   case llvm::omp::Directive::OMPD_target_data:
   case llvm::omp::Directive::OMPD_task:
+  case llvm::omp::Directive::OMPD_taskgroup:
   case llvm::omp::Directive::OMPD_teams:
   case llvm::omp::Directive::OMPD_workshare:
   case llvm::omp::Directive::OMPD_parallel_workshare:
   case llvm::omp::Directive::OMPD_target_teams:
   case llvm::omp::Directive::OMPD_target_parallel:
     PushContext(beginDir.source, beginDir.v);
     break;
   default:

 void OmpAttributeVisitor::Post(const parser::OpenMPBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
   switch (beginDir.v) {
   case llvm::omp::Directive::OMPD_parallel:
   case llvm::omp::Directive::OMPD_single:
   case llvm::omp::Directive::OMPD_target:
   case llvm::omp::Directive::OMPD_task:
+  case llvm::omp::Directive::OMPD_taskgroup:
   case llvm::omp::Directive::OMPD_teams:
   case llvm::omp::Directive::OMPD_parallel_workshare:
   case llvm::omp::Directive::OMPD_target_teams:
   case llvm::omp::Directive::OMPD_target_parallel: {
     bool hasPrivate;
     for (const auto *allocName : allocateNames_) {
       hasPrivate = false;
       for (auto privateObj : privateDataSharingAttributeObjects_) {

flang/lib/Semantics/resolve-names.cpp

 
 bool OmpVisitor::NeedsScope(const parser::OpenMPBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
   switch (beginDir.v) {
   case llvm::omp::Directive::OMPD_target_data:
   case llvm::omp::Directive::OMPD_master:
   case llvm::omp::Directive::OMPD_ordered:
+  case llvm::omp::Directive::OMPD_taskgroup:
     return false;
   default:
     return true;
   }
 }
 
 void OmpVisitor::AddOmpSourceRange(const parser::CharBlock &source) {
   messageHandler().set_currStmtSource(source);

flang/test/Semantics/omp-taskgroup01.f90

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+use omp_lib
+  implicit none
+  integer :: xyz, abc
+  !$omp parallel num_threads(4)
+    !$omp single
+      print *, "The"
+    !$omp taskgroup
+      !$omp task
+      print *, "almighty"
+      !$omp end task
+      !$omp task
+      print *, "sun"
+      !$omp end task
+    !$omp end taskgroup
+    !$omp end single
+  !$omp end parallel
+
+  !$omp parallel private(xyz)
+    !$omp taskgroup allocate(xyz)
+      !$omp task
+        print *, "The "
+        !$omp taskgroup allocate(omp_large_cap_mem_space: abc)
+          !$omp task
+          print *, "almighty sun"
+          !$omp end task
+        !$omp end taskgroup
+      !$omp end task
+    !$omp end taskgroup
+  !$omp end parallel
+
+  !ERROR: PRIVATE clause is not allowed on the TASKGROUP directive
+  !$omp taskgroup private(abc)
+  !$omp end taskgroup
+
+!  !$omp parallel
+!    !$omp taskgroup task_reduction(+, reduction_var)
+!      print *, "The "
+!      !$omp taskgroup task_reduction(min, reduction_var)
+!      !$omp taskgroup task_reduction(min, reduction_var)
+!      !$omp taskgroup task_reduction(.or., reduction_var)
+!      !$omp taskgroup task_reduction(iand, reduction_var)
+!        print *, "almighty sun"
+!      !$omp end taskgroup
+!      !$omp end taskgroup
+!      !$omp end taskgroup
+!      !$omp end taskgroup
+!      !$omp end task
+!      !$omp taskgroup task_reduction(-, xyz) task_reduction(-, xyz)
+!      !$omp end taskgroup
+!    !$omp end taskgroup
+!  !$omp end parallel
+
+
+end program

flang/test/Semantics/omp-taskloop01.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.9.2 taskloop Construct
 ! Assert fail for correct test case.
 
 subroutine parallel_work
   integer i
   integer j