Outermost OpenMP do-loop and cycle fails to compile

[Leporacanthicus]

As per comment in #1178:

program lineloop
  implicit none
  INTEGER ::  n
  
  !$OMP PARALLEL DO NUM_THREADS(2) SCHEDULE(STATIC,1) PRIVATE(n)
  DO n = 1, 2
     IF ( n > 2 )  CYCLE
  END DO
  !$OMP END PARALLEL DO
end program lineloop

fails with an assertion:

bbc: /data/f18-llvm-project/flang/lib/Lower/OpenMP.cpp:117: void createEmptyRegionBlocks(fir::FirOpBuilder &, std::list<Fortran::lower::pft::Evaluation> &): Assertion `eval.block->empty() && "block is not empty"' failed.

[vdonaldson]

This code compiles and executes correctly with the two changes below (on top of the PR #1178 changes). The change to createEmptyRegionBlocks takes care of the "missing terminator op" problem. The change in PFTBuilder.cpp to function exitConstructOrDirective provides a target node for the loop exit branch.

The problem is that at least the createEmptyRegionBlocks change is probably only applicable in some circumstances. As is, it breaks other tests. It probably needs to be done conditionally. The exitConstructOrDirective change is probably unilaterally ok, although there is some question as to whether the ContinueStmt should be inserted before or after the OmpEndLoopDirective Evaluation. (It may be that either way is ok.)

diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index af2d52d..e3a780b 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -112,11 +112,11 @@ void createEmptyRegionBlocks(
         // FIXME: Some subset of cases may need to insert a branch,
         // although this could be handled elsewhere.
         // if (?) {
-        //   auto insertPt = firOpBuilder.saveInsertionPoint();
-        //   firOpBuilder.setInsertionPointAfter(region->getParentOp());
-        //   firOpBuilder.create<mlir::BranchOp>(
-        //       terminatorOp.getLoc(), eval.block);
-        //   firOpBuilder.restoreInsertionPoint(insertPt);
+      /**/   auto insertPt = firOpBuilder.saveInsertionPoint();
+      /**/   firOpBuilder.setInsertionPointAfter(region->getParentOp());
+      /**/   firOpBuilder.create<mlir::BranchOp>(
+      /**/       terminatorOp.getLoc(), eval.block);
+      /**/   firOpBuilder.restoreInsertionPoint(insertPt);
         // }
       }
     }

diff --git a/flang/lib/Lower/PFTBuilder.cpp b/flang/lib/Lower/PFTBuilder.cpp
index 84df72d..a0ebefc 100644
--- a/flang/lib/Lower/PFTBuilder.cpp
+++ b/flang/lib/Lower/PFTBuilder.cpp
@@ -315,7 +315,8 @@ private:
       // A construct at the end of an (unstructured) OpenACC or OpenMP
       // construct region must have an exit target inside the region.
       auto &evaluationList = *eval->evaluationList;
-      if (!evaluationList.empty() && evaluationList.back().isConstruct()) {
+      if (!evaluationList.empty() && (evaluationList.back().isConstruct() ||
+                                      evaluationList.back().isDirective())) {
         static const parser::ContinueStmt exitTarget{};
         addEvaluation(
             lower::pft::Evaluation{exitTarget, pftParentStack.back(), {}, {}});

There are some other options for resolving these issues with nested constructs. Inserting TerminatorOp/YieldOp terminators in advance has both advantages and disadvantages for unstructured code variants. There is a chance that delaying that insertion could simplify other choices. Or not.

[Leporacanthicus]

I've used the posted changes, and they do indeed fix the original problem, but two of the existing tests fail (with an assert due to a block being incorrectly formed).

The test is the second part of flang/test/Lower/OpenMP/omp-unstructured.f90, subroutine ss3.

When setting a breakpoint at the bottom half of createBodyOfOp, just after the omp terminator or yield is instered, and printing the mlir, I get the following:

  %3 = fir.alloca i32 {adapt.valuebyref, pinned}
  %4 = fir.alloca i32 {adapt.valuebyref, pinned}
  %5 = fir.alloca i32 {pinned}
  %6 = fir.alloca i32 {bindc_name = "i", pinned}
  %c1_i32 = arith.constant 1 : i32
  %c3_i32 = arith.constant 3 : i32
  %c1_i32_0 = arith.constant 1 : i32
  %7 = arith.subi %c3_i32, %c1_i32 : i32
  %8 = arith.addi %7, %c1_i32_0 : i32
  %9 = arith.divsi %8, %c1_i32_0 : i32
  fir.store %9 to %5 : !fir.ref<i32>
  fir.store %c1_i32 to %6 : !fir.ref<i32>
  br ^bb1
^bb1:  // pred: ^bb0
  %10 = fir.load %5 : !fir.ref<i32>
  %c0_i32 = arith.constant 0 : i32
  %11 = arith.cmpi sgt, %10, %c0_i32 : i32
  cond_br %11, ^bb2, ^bb3
^bb2:  // pred: ^bb1
  %c1_i32_1 = arith.constant 1 : i32
  %c3_i32_2 = arith.constant 3 : i32
  %c1_i32_3 = arith.constant 1 : i32
  omp.wsloop (%arg1) : i32 = (%c1_i32_1) to (%c3_i32_2) inclusive step (%c1_i32_3)  {
    fir.store %arg1 to %4 : !fir.ref<i32>
    %c-1_i32 = arith.constant -1 : i32
    %12 = fir.address_of(@_QQcl.e69ca90633860f39f47b681aef9f34b5) : !fir.ref<!fir.char<1,72>>
    %c72 = arith.constant 72 : index
    %13 = fir.convert %12 : (!fir.ref<!fir.char<1,72>>) -> !fir.ref<i8>
    %c93_i32 = arith.constant 93 : i32
    %14 = fir.call @_FortranAioBeginExternalListOutput(%c-1_i32, %13, %c93_i32) : (i32, !fir.ref<i8>, i32) -> !fir.ref<i8>
    %15 = fir.address_of(@_QQcl.7373332D41) : !fir.ref<!fir.char<1,5>>
    %c5 = arith.constant 5 : index
    %16 = fir.convert %15 : (!fir.ref<!fir.char<1,5>>) -> !fir.ref<i8>
    %17 = fir.convert %c5 : (index) -> i64
    %18 = fir.call @_FortranAioOutputAscii(%14, %16, %17) : (!fir.ref<i8>, !fir.ref<i8>, i64) -> i1
    %19 = fir.load %4 : !fir.ref<i32>
    %20 = fir.call @_FortranAioOutputInteger32(%14, %19) : (!fir.ref<i8>, i32) -> i1
    %21 = fir.call @_FortranAioEndIoStatement(%14) : (!fir.ref<i8>) -> i32
    omp.yield
  }
  %c1_i32_4 = arith.constant 1 : i32
  %c3_i32_5 = arith.constant 3 : i32
  %c1_i32_6 = arith.constant 1 : i32
  omp.wsloop (%arg1) : i32 = (%c1_i32_4) to (%c3_i32_5) inclusive step (%c1_i32_6)  {
    fir.store %arg1 to %3 : !fir.ref<i32>
  ^bb1:  // no predecessors
  ^bb2:  // no predecessors
  ^bb3:  // no predecessors
  ^bb4:  // no predecessors
  ^bb5:  // no predecessors
  ^bb6:  // no predecessors
    omp.yield
  }
^bb3:  // pred: ^bb1
  omp.terminator
}

Which, given where we are, looks OK.

Continuing from there, we end up in mlir::block::getTerminator which complains that the block doesn't have a terminator. That's at Block.cpp:258 - moving back up the stack a couple of layers, we now have a block like this:
^bb6: // 2 preds: ^bb2, ^bb4
which is inside this outer block:

  fir.store %arg1 to %5 : !fir.ref<i32>
  br ^bb1
^bb1:  // pred: ^bb0
  %c1_i32_8 = arith.constant 1 : i32
  %c3_i32_9 = arith.constant 3 : i32
  %c1_i32_10 = arith.constant 1 : i32
  %18 = arith.subi %c3_i32_9, %c1_i32_8 : i32
  %19 = arith.addi %18, %c1_i32_10 : i32
  %20 = arith.divsi %19, %c1_i32_10 : i32
  fir.store %20 to %3 : !fir.ref<i32>
  fir.store %c1_i32_8 to %4 : !fir.ref<i32>
  br ^bb2
^bb2:  // 2 preds: ^bb1, ^bb5
  %21 = fir.load %3 : !fir.ref<i32>
  %c0_i32_11 = arith.constant 0 : i32
  %22 = arith.cmpi sgt, %21, %c0_i32_11 : i32
  cond_br %22, ^bb3, ^bb6
^bb3:  // pred: ^bb2
  %23 = fir.load %4 : !fir.ref<i32>
  %24 = fir.load %arg0 : !fir.ref<i32>
  %25 = arith.cmpi eq, %23, %24 : i32
  cond_br %25, ^bb4, ^bb5
^bb4:  // pred: ^bb3
  br ^bb6
^bb5:  // pred: ^bb3
  %c-1_i32 = arith.constant -1 : i32
  %26 = fir.address_of(@_QQcl.e69ca90633860f39f47b681aef9f34b5) : !fir.ref<!fir.char<1,72>>
  %c72 = arith.constant 72 : index
  %27 = fir.convert %26 : (!fir.ref<!fir.char<1,72>>) -> !fir.ref<i8>
  %c100_i32 = arith.constant 100 : i32
  %28 = fir.call @_FortranAioBeginExternalListOutput(%c-1_i32, %27, %c100_i32) : (i32, !fir.ref<i8>, i32) -> !fir.ref<i8>
  %29 = fir.address_of(@_QQcl.7373332D42) : !fir.ref<!fir.char<1,5>>
  %c5 = arith.constant 5 : index
  %30 = fir.convert %29 : (!fir.ref<!fir.char<1,5>>) -> !fir.ref<i8>
  %31 = fir.convert %c5 : (index) -> i64
  %32 = fir.call @_FortranAioOutputAscii(%28, %30, %31) : (!fir.ref<i8>, !fir.ref<i8>, i64) -> i1
  %33 = fir.load %4 : !fir.ref<i32>
  %34 = fir.call @_FortranAioOutputInteger32(%28, %33) : (!fir.ref<i8>, i32) -> i1
  %35 = fir.call @_FortranAioEndIoStatement(%28) : (!fir.ref<i8>) -> i32
  %36 = fir.load %3 : !fir.ref<i32>
  %c1_i32_12 = arith.constant 1 : i32
  %37 = arith.subi %36, %c1_i32_12 : i32
  fir.store %37 to %3 : !fir.ref<i32>
  %38 = fir.load %4 : !fir.ref<i32>
  %39 = arith.addi %38, %c1_i32_10 : i32
  fir.store %39 to %4 : !fir.ref<i32>
  br ^bb2
^bb6:  // 2 preds: ^bb2, ^bb4
}

As can be seen, the last block has lost the omp.yield instruction.

I have not been able to find where that goes missing yet.

[Leporacanthicus]

So, I have been working on a couple of other things for a few days, but I think what needs to happen is that the if ( ) that is commented out in the original parttal fix needs to check that the unstructured code is in the outermost loop, rather than in an inner loop (which is what fails in the existing tests when applying the partial fix.

I'm looking at that right now.

[Leporacanthicus]

The above comment is wrong - or at least I think so. The first part of the patch works fine with no if-condition, it doesn't cause any issues.

The problem is that if we add an exitTarget to directives, it fails for some cases, and passes for others.

I tried to find a way to determine the different cases - I added a static int variable and skipped certain counts, and with that I can make both tests pass - but that's obviously not going to work for arbitrary code. I have not been able to find a solution to tell the difference between the case that works and the case that fails. I think the difference is that the failing case the exit is in an inner loop, the passing case the exit is in the outer loop.

I'm pausing work on this for now.

[kiranchandramohan]

@vdonaldson

The problem is that at least the createEmptyRegionBlocks change is probably only applicable in some circumstances. As is, it breaks other tests. It probably needs to be done conditionally.

Is this in nested OpenMP contexts? Like, should it be applicable only to the innermost OpenMP construct enclosing the unstructured code?

The exitConstructOrDirective change is probably unilaterally ok, although there is some question as to whether the ContinueStmt should be inserted before or after the OmpEndLoopDirective Evaluation. (It may be that either way is ok.)

Currently, I have seen cases where the terminator of the outer OpenMP region is present in the continue of the inner region. This probably happens because the outer region's continue has no block corresponding to it but the inner OpenMP region's continue block is and is the last one that is processed. I kind of get the feeling that at the moment the OpenMP terminators are added to the last processed block in createEmptyRegionBlocks. Shall we modify createEmptyRegionBlocks to also insert the terminator? Will it have enough information on where it should be added?

In the following case, a branch was missing from the x = j - 222 block to the block with the omp.yield. Is adding a branch here the right approach? Alternatively, we can insert the omp.yield itself into the x = j - 222 block if we can find that it is the last block in the !$omp do region.

  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

I was also wondering what is the right place to insert the OpenMP region terminators. Since this is not something that is handled by the PFT or other regular FIR code generation. Is the continue block that is artificially added a good place for inserting the terminators. Alternatively, should we always split the block inside an OpenMP region and always have the form like the one below.

omp.op {
...
<structured or unstructured code>
...
bbN:
  omp.terminator
}

[kiranchandramohan]

I have created a PR that fixes this issue. #1394
@vdonaldson please have a look.

[kiranchandramohan]

Issues fixed by the patch mentioned above.