[flang-commits] [llvm] [clang] [clang-tools-extra] [flang] [compiler-rt] [Flang][OpenMP] Restructure recursive lowering in `createBodyOfOp` (PR #77761)

[Krzysztof Parzyszek via flang-commits]

================
@@ -2223,37 +2247,64 @@ static void createBodyOfOp(
                                             mlir::omp::YieldOp>(
         firOpBuilder, eval.getNestedEvaluations());
 
-  // Insert the terminator.
-  Fortran::lower::genOpenMPTerminator(firOpBuilder, op.getOperation(), loc);
-  // Reset the insert point to before the terminator.
-  resetBeforeTerminator(firOpBuilder, storeOp, block);
+  // Start with privatization, so that the lowering of the nested
+  // code will use the right symbols.
+  constexpr bool isLoop = std::is_same_v<Op, mlir::omp::WsLoopOp> ||
+                          std::is_same_v<Op, mlir::omp::SimdLoopOp>;
+  bool privatize = clauses && !outerCombined;
 
-  // Handle privatization. Do not privatize if this is the outer operation.
-  if (clauses && !outerCombined) {
-    constexpr bool isLoop = std::is_same_v<Op, mlir::omp::WsLoopOp> ||
-                            std::is_same_v<Op, mlir::omp::SimdLoopOp>;
+  firOpBuilder.setInsertionPoint(marker);
+  std::optional<DataSharingProcessor> tempDsp;
+  if (privatize) {
     if (!dsp) {
-      DataSharingProcessor proc(converter, *clauses, eval);
-      proc.processStep1();
-      proc.processStep2(op, isLoop);
-    } else {
-      if (isLoop && args.size() > 0)
-        dsp->setLoopIV(converter.getSymbolAddress(*args[0]));
-      dsp->processStep2(op, isLoop);
+      tempDsp.emplace(converter, *clauses, eval);
+      tempDsp->processStep1();
     }
-
-    if (storeOp)
-      firOpBuilder.setInsertionPointAfter(storeOp);
   }
 
   if constexpr (std::is_same_v<Op, mlir::omp::ParallelOp>) {
     threadPrivatizeVars(converter, eval);
-    if (clauses)
+    if (clauses) {
+      firOpBuilder.setInsertionPoint(marker);
       ClauseProcessor(converter, *clauses).processCopyin();
+    }
   }
 
-  if (genNested)
+  if (genNested) {
+    // genFIR(Evaluation&) tries to patch up unterminated blocks, causing
+    // a lot of trouble if the terminator generation is delayed past this
+    // point. Insert a temporary terminator here, then delete it.
+    firOpBuilder.setInsertionPointToEnd(&op.getRegion().back());
+    auto *temp = Fortran::lower::genOpenMPTerminator(firOpBuilder,
+                                                     op.getOperation(), loc);
+    firOpBuilder.setInsertionPointAfter(marker);
     genNestedEvaluations(converter, eval);
+    temp->erase();
+  }
+
+  if (auto *exitBlock = findExitBlock(op.getRegion())) {
+    firOpBuilder.setInsertionPointToEnd(exitBlock);
+    auto *term = Fortran::lower::genOpenMPTerminator(firOpBuilder,
+                                                     op.getOperation(), loc);
+    // Only insert lastprivate code when there actually is an exit block.
+    // Such a block may not exist if the nested code produced an infinite
+    // loop (this may not make sense in production code, but a user could
+    // write that and we should handle it).
----------------
kparzysz wrote:

We rely on the `genEval` for the nested code to do its thing.  If we consider the nested code (PFT/Evaluation) on its own, it will have a single entry point, and zero or more exit points.  If it creates multiple blocks, it will need to create its own branches to facilitate the control flow.  After `genEval` we remove the temporary terminator, then we look at all the blocks created during `genEval`.  We know that everything that is present inside of these blocks was created for the nested code (and not the enclosing construct we're lowering).  Terminators for our op haven't been created yet, and they can only placed in blocks that don't yet have a terminator.  If every block has a terminator then there is no way to leave the region by means of reaching the end of it (that is aside from function calls).

Based on that, the logical thing to do would be to insert a terminator into all non-terminated blocks.  This would work even if there is always at most one such block.

The assertion that there can only be one comes mostly from two things:
- the requirement from the OpenMP standard that a structured block exits at the bottom of the program text, and
- the current lowering seems to provide a single FIR location that corresponds to the "bottom".

If we can't be sure that the assertion is correct, we should be able to remove it, and process all exiting blocks.

https://github.com/llvm/llvm-project/pull/77761