[Flang][OpenMP] Fix for unstructured regions in OpenMP constructs - 2

[kiranchandramohan]

Summary

The following changes are made for OpenMP operations with unstructured region,

1. For combined constructs the outer operation is considered a structured

region and the inner one as the unstructured.

2. Added a condition to ensure that we create new blocks only once for nested

unstructured OpenMP constructs.

Tests are added for checking the structure of the CFG.

Note: This is part of upstreaming from the fir-dev branch of
https://github.com/flang-compiler/f18-llvm-project. Code originally reviewed
at https://github.com/flang-compiler/f18-llvm-project/pull/1394.

[vdonaldson]

Looks ok to me

[shraiysh]

LGTM. I can see that one condition has been added for the first point in patch summary. I am unable to spot the condition for the second point, can you please tell me where that check happens?

[kiranchandramohan]

flang/lib/Lower/OpenMP.cpp

159

@shraiysh the second check is here.

[peixin]

LGTM

[shraiysh]

flang/lib/Lower/OpenMP.cpp

159

This check checks that we did it only for constructs but this doesn't check for the "only once" condition right? I thought that was the emphasis for the second point.

[kiranchandramohan]

flang/lib/Lower/OpenMP.cpp

159

createEmptyRegionBlocks is called for all OpenMP regions from createBodyOfOp. When there are nested constructs, like a do inside a parallel, without this check, it will be called two times for the doregion, first the nested case from parallel and second the direct case for do.

So if this check is not there it will be effectively called more than once.

[schweitz]

flang/test/Lower/OpenMP/omp-unstructured.f90

126

Wasn't there some work to turn these sorts of guards into structured code?

276

Shouldn't there be an omp.wsloop near here? Do you want to verify that it is there?

[vdonaldson]

flang/test/Lower/OpenMP/omp-unstructured.f90

126

Subroutine ss4 initially generates structured fir in the fir sense of the term. Since the RUN directive for the test does not specify emit-fir, the CHECK directives apply to an mlir file that has explicit basic blocks. That may not matter here. Output for "-fopenmp -fdebug-dump-pre-fir" shows structured code. With "-emit-fir", the mlir file does as well.

1 Subroutine ss4: subroutine ss4(n)
  1 <<OpenMPConstruct>>
    <<DoConstruct>> -> 11
      2 NonLabelDoStmt -> 10: do i = 1, 3
      3 <<^OpenMPConstruct>>
        <<DoConstruct>> -> 10
          4 NonLabelDoStmt -> 9: do j = 1, 3
          <<IfConstruct>> -> 9
            5 ^IfStmt [negate] -> 9: if(j .eq. n) cycle
            8 ^PrintStmt: print*, 'ss4', j
            7 EndIfStmt
          <<End IfConstruct>>
          9 EndDoStmt -> 4: enddo
        <<End DoConstruct>>
        OmpEndLoopDirective
      <<End OpenMPConstruct>>
      10 EndDoStmt -> 2: enddo
    <<End DoConstruct>>
    11 ContinueStmt
  <<End OpenMPConstruct>>
  12 EndSubroutineStmt: end
End Subroutine ss4

[shraiysh]

flang/lib/Lower/OpenMP.cpp

159

Okay, understood. Thanks for clarifying.

[kiranchandramohan]

I am going ahead with this patch. If there are further comments then will address in a separate patch.

flang/test/Lower/OpenMP/omp-unstructured.f90

126

Thanks @vdonaldson for clarifying. It does not matter here, we are checking that all these cases generate correct code and do not crash.

276

@schweitz In ss9 we only have a parallel and not a parallel do, so there is no omp.wsloop.

flang/lib/Lower/OpenMP.cpp

if (eval.block) {
         eval.block = firOpBuilder.createBlock(region);
       } else {
         [[maybe_unused]] auto &terminatorOp = eval.block->back();
         assert((mlir::isa<mlir::omp::TerminatorOp>(terminatorOp) ||
                 mlir::isa<mlir::omp::YieldOp>(terminatorOp)) &&
                "expected terminator op");
       }
     }
-    if (eval.hasNestedEvaluations())
+    if (!eval.isDirective() && eval.hasNestedEvaluations())
       createEmptyRegionBlocks(firOpBuilder, eval.getNestedEvaluations());
   }
 }
 
 /// Create the body (block) for an OpenMP Operation.
 ///
 /// \param [in]    op - the operation the body belongs to.
 /// \param [inout] converter - converter to use for the clauses.
 /// \param [in]    loc - location in source code.
 /// \param [in]    eval - current PFT node/evaluation.
 /// \oaran [in]    clauses - list of clauses to process.
 /// \param [in]    args - block arguments (induction variable[s]) for the
 ////                      region.
 /// \param [in]    outerCombined - is this an outer operation - prevents
 ///                                privatization.
 template <typename Op>
 static void
 createBodyOfOp(Op &op, Fortran::lower::AbstractConverter &converter,
                mlir::Location &loc, Fortran::lower::pft::Evaluation &eval,
                const Fortran::parser::OmpClauseList *clauses = nullptr,
                const SmallVector<const Fortran::semantics::Symbol *> &args = {},
                bool outerCombined = false) {
-  auto &firOpBuilder = converter.getFirOpBuilder();
+  fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   // If an argument for the region is provided then create the block with that
   // argument. Also update the symbol's address with the mlir argument value.
   // e.g. For loops the argument is the induction variable. And all further
   // uses of the induction variable should use this mlir value.
   if (args.size()) {
     std::size_t loopVarTypeSize = 0;
     for (const Fortran::semantics::Symbol *arg : args)
       loopVarTypeSize = std::max(loopVarTypeSize, arg->GetUltimate().size());
if (args.size()) {
     for (const Fortran::semantics::Symbol *arg : args) {
       fir::ExtendedValue exval = op.getRegion().front().getArgument(argIndex);
       converter.bindSymbol(*arg, exval);
       argIndex++;
     }
   } else {
     firOpBuilder.createBlock(&op.getRegion());
   }
-  auto &block = op.getRegion().back();
-  firOpBuilder.setInsertionPointToStart(&block);
+  mlir::Block &block = op.getRegion().back();
+  firOpBuilder.setInsertionPointToEnd(&block);
 
-  if (eval.lowerAsUnstructured())
+  // If it is an unstructured region and is not the outer region of a combined
+  // construct, create empty blocks for all evaluations.
+  if (eval.lowerAsUnstructured() && !outerCombined)
     createEmptyRegionBlocks(firOpBuilder, eval.getNestedEvaluations());
 
-  // Ensure the block is well-formed by inserting terminators.
+  // Insert the terminator.
   if constexpr (std::is_same_v<Op, omp::WsLoopOp>) {
     mlir::ValueRange results;
     firOpBuilder.create<mlir::omp::YieldOp>(loc, results);
   } else {
     firOpBuilder.create<mlir::omp::TerminatorOp>(loc);
   }
 
   // Reset the insertion point to the start of the first block.
   firOpBuilder.setInsertionPointToStart(&block);
+
   // Handle privatization. Do not privatize if this is the outer operation.
   if (clauses && !outerCombined)
     privatizeVars(converter, *clauses);
 }
 
 static void genOMP(Fortran::lower::AbstractConverter &converter,
                    Fortran::lower::pft::Evaluation &eval,
                    const Fortran::parser::OpenMPSimpleStandaloneConstruct

flang/test/Lower/OpenMP/omp-unstructured.f90

do i = 1, 3
             print*, 'ss3-B', k
           enddo
         enddo
       !$omp end do
     enddo
   !$omp end parallel
 end
 
+! CHECK-LABEL: func @_QPss4{{.*}} {
+! CHECK:       omp.parallel {
+! CHECK:         omp.wsloop for (%[[ARG:.*]]) : {{.*}} {
+! CHECK:           cond_br %{{.*}}, ^bb1, ^bb2
+! CHECK:          ^bb1:
+! CHECK:           @_FortranAioBeginExternalListOutput
+! CHECK:           @_FortranAioOutputInteger32(%{{.*}}, %[[ARG]])
+! CHECK:           br ^bb2
+! CHECK:         ^bb2:
+! CHECK-NEXT:      omp.yield
+! CHECK-NEXT:  }
+! CHECK:       omp.terminator
+! CHECK-NEXT:}
+subroutine ss4(n) ! CYCLE in OpenMP wsloop constructs
+  !$omp parallel
+    do i = 1, 3
+      !$omp do
+        do j = 1, 3
+           if (j .eq. n) cycle
+           print*, 'ss4', j
+        enddo
+      !$omp end do
+    enddo
+  !$omp end parallel
+end
+
+! CHECK-LABEL: func @_QPss5() {
+! CHECK:  omp.parallel  {
+! CHECK:    omp.wsloop {{.*}} {
+! CHECK:      br ^[[BB1:.*]]
+! CHECK:    ^[[BB1]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB2:.*]], ^[[BB4:.*]]
+! CHECK:    ^[[BB2]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB4]], ^[[BB3:.*]]
+! CHECK:    ^[[BB3]]:
+! CHECK:      br ^[[BB1]]
+! CHECK:    ^[[BB4]]:
+! CHECK:      omp.yield
+! CHECK:    }
+! CHECK:    omp.terminator
+! CHECK:  }
+subroutine ss5() ! EXIT inside OpenMP wsloop (inside parallel)
+  integer :: x
+  !$omp parallel private(x)
+    !$omp do
+      do j = 1, 3
+        x = j * i
+        do k = 1, 3
+          if (k .eq. n) exit
+          x = k
+          x = x + k
+        enddo
+        x = j - 222
+      enddo
+    !$omp end do
+  !$omp end parallel
+end
+
+! CHECK-LABEL: func @_QPss6() {
+! CHECK:  omp.parallel  {
+! CHECK:    br ^[[BB1_OUTER:.*]]
+! CHECK:  ^[[BB1_OUTER]]:
+! CHECK:    cond_br %{{.*}}, ^[[BB2_OUTER:.*]], ^[[BB3_OUTER:.*]]
+! CHECK:  ^[[BB2_OUTER]]:
+! CHECK:    omp.wsloop {{.*}} {
+! CHECK:      br ^[[BB1:.*]]
+! CHECK:    ^[[BB1]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB2:.*]], ^[[BB4:.*]]
+! CHECK:    ^[[BB2]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB4]], ^[[BB3:.*]]
+! CHECK:    ^[[BB3]]:
+! CHECK:      br ^[[BB1]]
+! CHECK:    ^[[BB4]]:
+! CHECK:      omp.yield
+! CHECK:    }
+! CHECK:    br ^[[BB1_OUTER]]
+! CHECK:  ^[[BB3_OUTER]]:
+! CHECK:    omp.terminator
+! CHECK:  }
+subroutine ss6() ! EXIT inside OpenMP wsloop in a do loop (inside parallel)
+  integer :: x
+  !$omp parallel private(x)
+    do i = 1, 3
+      !$omp do
+        do j = 1, 3
+          x = j * i
+          do k = 1, 3
+            if (k .eq. n) exit
+            x = k
+            x = x + k
+          enddo
+          x = j - 222
+        enddo
+      !$omp end do
+    enddo
+  !$omp end parallel
+end
+
+! CHECK-LABEL: func @_QPss7() {
+! CHECK: br ^[[BB1_OUTER:.*]]
+! CHECK: ^[[BB1_OUTER]]:
+! CHECK:   cond_br %{{.*}}, ^[[BB2_OUTER:.*]], ^[[BB3_OUTER:.*]]
+! CHECK-NEXT: ^[[BB2_OUTER:.*]]:
+! CHECK:   omp.parallel  {
+! CHECK:     omp.wsloop {{.*}} {
+! CHECK:       br ^[[BB1:.*]]
+! CHECK-NEXT:     ^[[BB1]]:
+! CHECK:       cond_br %{{.*}}, ^[[BB2:.*]], ^[[BB4:.*]]
+! CHECK-NEXT:     ^[[BB2]]:
+! CHECK:       cond_br %{{.*}}, ^[[BB4]], ^[[BB3:.*]]
+! CHECK-NEXT:     ^[[BB3]]:
+! CHECK:       br ^bb1
+! CHECK-NEXT:     ^[[BB4]]:
+! CHECK:       omp.yield
+! CHECK:     }
+! CHECK:     omp.terminator
+! CHECK:   }
+! CHECK:   br ^[[BB1_OUTER]]
+! CHECK-NEXT: ^[[BB3_OUTER]]:
+! CHECK-NEXT:   return
+subroutine ss7() ! EXIT inside OpenMP parallel do (inside do loop)
+  integer :: x
+    do i = 1, 3
+      !$omp parallel do private(x)
+        do j = 1, 3
+          x = j * i
+          do k = 1, 3
+            if (k .eq. n) exit
+            x = k
+            x = x + k
+          enddo
+        enddo
+      !$omp end parallel do
+    enddo
+end
+
+! CHECK-LABEL: func @_QPss8() {
+! CHECK:  omp.parallel  {
+! CHECK:    omp.wsloop {{.*}} {
+! CHECK:      br ^[[BB1:.*]]
+! CHECK:    ^[[BB1]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB2:.*]], ^[[BB4:.*]]
+! CHECK:    ^[[BB2]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB4]], ^[[BB3:.*]]
+! CHECK:    ^[[BB3]]:
+! CHECK:      br ^[[BB1]]
+! CHECK:    ^[[BB4]]:
+! CHECK:      omp.yield
+! CHECK:    }
+! CHECK:    omp.terminator
+! CHECK:  }
+subroutine ss8() ! EXIT inside OpenMP parallel do
+  integer :: x
+      !$omp parallel do private(x)
+        do j = 1, 3
+          x = j * i
+          do k = 1, 3
+            if (k .eq. n) exit
+            x = k
+            x = x + k
+          enddo
+        enddo
+      !$omp end parallel do
+end
+
+! CHECK-LABEL: func @_QPss9() {
+! CHECK:  omp.parallel  {
+! CHECK-NEXT:    omp.parallel  {
+! CHECK:      br ^[[BB1:.*]]
+! CHECK:         ^[[BB1]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB2:.*]], ^[[BB4:.*]]
+! CHECK-NEXT:    ^[[BB2]]:
+! CHECK:      cond_br %{{.*}}, ^[[BB4]], ^[[BB3:.*]]
+! CHECK-NEXT:    ^[[BB3]]:
+! CHECK:      br ^[[BB1]]
+! CHECK-NEXT:    ^[[BB4]]:
+! CHECK:      omp.terminator
+! CHECK-NEXT:    }
+! CHECK:    omp.terminator
+! CHECK-NEXT  }
+! CHECK: }
+subroutine ss9() ! EXIT inside OpenMP parallel (inside parallel)
+  integer :: x
+  !$omp parallel
+  !$omp parallel private(x)
+    do k = 1, 3
+      if (k .eq. n) exit
+      x = k
+      x = x + k
+    end do
+  !$omp end parallel
+  !$omp end parallel
+end
+
 ! CHECK-LABEL: func @_QQmain
 program p
   call ss1(2)
   call ss2(2)
   call ss3(2)
+  call ss4(2)
+  call ss5()
+  call ss6()
+  call ss7()
+  call ss8()
+  call ss9()
 end