[PATCH] D106538: [flang][OpenMP] Add semantic check for cancellation nesting

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

In D106538#2896051 <https://reviews.llvm.org/D106538#2896051>, @peixin wrote:

> Currently, I do not implement the semantic check for the following two scenarios, which should not be allowed according to OpenMP Version 5.0 Specification.
>
>   !$omp task
>     !$omp cancel taskgroup
>   !$omp end task
>
>   !$omp taskloop nogroup
>     do i = 1, N
>       !$omp cancel taskgroup
>       a = 3.14;
>     end do
>
> They are supported by gfortran and current clang. Should we report errors or warnings or just ignore it? Or maybe my understanding is incorrect.

The region is a dynamic concept, unlike constructs. So nesting of regions is satisfied if the taskgroup is enclosing somewhere in the call sequence. This is difficult to test since it requires interprocedural analysis.

  subroutine two
  !$omp task
    !$omp cancel taskgroup
  !$omp end task
  end subroutine two
  
  subroutine one
  !$omp taskgroup
  call two()
  !$omp end taskgroup
  end subroutine one

I think the current implementation does not catch the following case which is clearly not permitted by the standard.
(Note the definition of closely nested region. A region nested inside another region with no parallel region nested between them)

  subroutine sb
  !$omp parallel
    !$omp task
      !$omp cancel taskgroup
    !$omp end task
  !$omp end parallel
  end subroutine



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:934
+  } else {
+    context_.Say(source, "orphaned %s directives are prohibited"_err_en_US,
+        ContextDirectiveAsFortran());
----------------
Nit: Can you provide a better error message here? Like "%s directive is not closely nested inside %s"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106538/new/

https://reviews.llvm.org/D106538