[Flang][PFT] Skip continue insertion for OpenMP Loops

[kiranchandramohan]

Summary

Unstructured regions presents some issues for OpenMP code generation.
While there are no branches out of the OpenMP region, there can be
branches inside. This required the availability of an artificial
target at the end of an OpenMP region. This was implemented by
insertion an artifical continue and marking it as a target for
a branch.
(https://github.com/flang-compiler/f18-llvm-project/pull/1178)

The artificial target is not required for OpenMP loops. Since the
DO loop end can itself be a target of a branch. Moreover, insertion
of the continue between the end of the loop and the end of the
OpenMP loop construct presents problems since the OpenMP MLIR
loop construct models both the loop and the construct. This can
cause the terminator of the OpenMP loop construct to be missed.
This patch solves the issue by skipping the insertion of the
continue.

Note: This issue is only hit if the end openmp loop directive
is missed.

This patch fixes the issues in:
-> https://github.com/llvm/llvm-project/issues/58378
-> https://github.com/flang-compiler/f18-llvm-project/issues/1426

Fixes #58378

[kiranchandramohan]

flang/lib/Lower/PFTBuilder.cpp

344

Another option could be to extend the check here for an End Do Construct.

[vdonaldson]

flang/lib/Lower/PFTBuilder.cpp

344

Checking for an EndDoStmt would have the advantage of also being applicable for OpenACC code.

[kiranchandramohan]

Thanks @vdonaldson for having a look.

flang/lib/Lower/PFTBuilder.cpp

344

Looking at this again, not sure whether checking for EndDoStmt or EndDoConstruct will work since there is insufficient context to distinguish between,

!$omp parallel
do i=1,10
if (cond) exit
end do
!$omp end parallel

!$omp do
do i=1,10
if (cond) cycle
end do

For the !$omp parallel case, the continue is required, whereas for the !$omp do case it is not required.

[vdonaldson]

@kiranchandramohan Thanks for investigating fix options.

As a side note, there is no End DoConstruct node in the PFT. A PFT dump tag without spaces, such as EndSubroutineStmt is a language syntax element with a corresponding PFT node. For a DO loop the reference is to f18 syntax rule R1119 in clause 11.1.7.1. Any dump line with "End ", with a space, is a hint/reminder that it is just a bracketing convenience for dump readers.

What you have looks ok to me for addressing the problem if you want to proceed with that.

[kiranchandramohan]

In D151700#4384626, @vdonaldson wrote:

@kiranchandramohan Thanks for investigating fix options.

As a side note, there is no End DoConstruct node in the PFT. A PFT dump tag without spaces, such as EndSubroutineStmt is a language syntax element with a corresponding PFT node. For a DO loop the reference is to f18 syntax rule R1119 in clause 11.1.7.1. Any dump line with "End ", with a space, is a hint/reminder that it is just a bracketing convenience for dump readers.

What you have looks ok to me for addressing the problem if you want to proceed with that.

Thanks @vdonaldson for the info.
I will proceed with this fix for now.

flang/lib/Lower/PFTBuilder.cpp

bool enterConstructOrDirective(const A &constructOrDirective) {
     eval.evaluationList.reset(new lower::pft::EvaluationList);
     pushEvaluationList(eval.evaluationList.get());
     pftParentStack.emplace_back(eval);
     constructAndDirectiveStack.emplace_back(&eval);
     return true;
   }
 
   void exitConstructOrDirective() {
+    auto isOpenMPLoopConstruct = [](Fortran::lower::pft::Evaluation *eval) {
+      if (const auto *ompConstruct = eval->getIf<parser::OpenMPConstruct>())
+        if (std::holds_alternative<parser::OpenMPLoopConstruct>(
+                ompConstruct->u))
+          return true;
+      return false;
+    };
+
     rewriteIfGotos();
     auto *eval = constructAndDirectiveStack.back();
-    if (eval->isExecutableDirective()) {
+    if (eval->isExecutableDirective() && !isOpenMPLoopConstruct(eval)) {
       // A construct at the end of an (unstructured) OpenACC or OpenMP
       // construct region must have an exit target inside the region.
+      // This is not applicable to the OpenMP loop construct since the
+      // end of the loop is an available target inside the region.
       Fortran::lower::pft::EvaluationList &evaluationList =
           *eval->evaluationList;
       if (!evaluationList.empty() && evaluationList.back().isConstruct()) {
         static const parser::ContinueStmt exitTarget{};
         addEvaluation(
             lower::pft::Evaluation{exitTarget, pftParentStack.back(), {}, {}});
       }
     }
     popEvaluationList();
     pftParentStack.pop_back();
     constructAndDirectiveStack.pop_back();

flang/test/Lower/OpenMP/pre-fir-tree-loop.f90

+! RUN: bbc -fopenmp -pft-test -o %t %s | FileCheck %s
+! RUN: %flang_fc1 -fopenmp -fdebug-dump-pft -o %t %s | FileCheck %s
+
+! Loop constructs always have an `end do` which can be the target of
+! a branch. So OpenMP loop constructs do not need an artificial
+! continue inserted for a target.
+
+!CHECK-LABEL: sb0
+!CHECK-NOT: continue
+subroutine sb0(cond)
+  implicit none
+  logical :: cond
+  integer :: i
+  !$omp parallel do
+  do i = 1, 20
+    if( cond) then
+      cycle
+    end if
+  end do
+  return
+end subroutine
+
+!CHECK-LABEL: sb1
+!CHECK-NOT: continue
+subroutine sb1(cond)
+  implicit none
+  logical :: cond
+  integer :: i
+  !$omp parallel do
+  do i = 1, 20
+    if( cond) then
+      cycle
+    end if
+  end do
+  !$omp end parallel do
+  return
+end subroutine
+
+!CHECK-LABEL: sb2
+!CHECK-NOT: continue
+subroutine sb2
+  integer :: i, n
+  integer :: tmp
+
+  !$omp parallel do
+  do ifld=1,n
+     do isum=1,n
+       if (tmp > n) then
+         exit
+       endif
+     enddo
+     tmp = n
+  enddo
+end subroutine
+
+!CHECK-LABEL: sb3
+!CHECK-NOT: continue
+subroutine sb3
+  integer :: i, n
+  integer :: tmp
+
+  !$omp parallel do
+  do ifld=1,n
+     do isum=1,n
+       if (tmp > n) then
+         exit
+       endif
+     enddo
+  enddo
+end subroutine