[Openmp-commits] [PATCH] D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.

[Kiran Chandramohan via Phabricator via Openmp-commits]

kiranchandramohan added a comment.

Thanks @Meinersbur for this patch. Apologies for the delay in reviewing many of your patches.

The patch looks mostly LGTM.  A few nits. The patch probably needs a rebase since there are no dependencies now and a few more construct lowering has landed in MLIR (single, simdloop).



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5571-5579
+          {
+            OMPBuilderCBHelpers::InlinedRegionBodyRAII IRB(*this, AllocaIP,
+                                                           *FiniBB);
+            EmitStmt(CS->getCapturedStmt());
+          }
+
+          if (Builder.saveIP().isSet())
----------------
Why is it not possible to use `OMPBuilderCBHelpers::EmitOMPInlinedRegionBody` here?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1819
+    /// Emit the body of an OMP region that will be outlined in
+    /// OpenMPIRBuilder::finialize().
+    /// \param CGF	          The Codegen function this belongs to
----------------
Nit:finalize


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:26-58
+/// Move the instruction after an InsertPoint to the beginning of another
+/// BasicBlock.
+///
+/// The instructions after \p IP are moved to the beginning of \p New which must
+/// not have any PHINodes. If \p CreateBranch is true, a branch instruction to
+/// \p New will be added such that there is no semantic change. Otherwise, the
+/// \p IP insert block remains degenerate and it is up to the caller to insert a
----------------
I guess these are already in from your other patch (https://reviews.llvm.org/D114413).


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:138
+  /// BasicBlock after CodeGenIP including CodeGenIP itself are invalidated. If
+  /// such InsertPoints need to be preserved, it can split the block itseff
+  /// before calling the callback.
----------------
Nit:itself


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1086
   auto FiniInfo = FinalizationStack.pop_back_val();
-  assert(FiniInfo.DK == OMPD_sections &&
-         "Unexpected finalization stack state!");
-  Builder.SetInsertPoint(LoopAfterBB->getTerminator());
-  FiniInfo.FiniCB(Builder.saveIP());
-  Builder.SetInsertPoint(ExitBB);
+  assert(FiniInfo.DK == OMPD_sections && "Uxpected finalization stack state!");
+  if (FinalizeCallbackTy &CB = FiniInfo.FiniCB) {
----------------
Nit:Unexpected


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:103
+  llvm::BasicBlock *continuationBlock =
+      splitBB(builder, true, "omp.region.cont");
+  llvm::BasicBlock *sourceBlock = builder.GetInsertBlock();
----------------
Would this "omp.region.cont" be a mostly empty block in most cases? I guess it is not a problem since llvm will remove these blocks.

The IR generated for 
```
  omp.parallel {
    omp.barrier
    omp.terminator
  }
```
is the following. Notice the empty (only a branch) `omp.region.cont` block. Previously we had this only at the source side `omp.par.region`.

```
omp.par.entry:
  %tid.addr.local = alloca i32, align 4
  %0 = load i32, i32* %tid.addr, align 4
  store i32 %0, i32* %tid.addr.local, align 4
  %tid = load i32, i32* %tid.addr.local, align 4
  br label %omp.par.region

omp.par.region:                                   ; preds = %omp.par.entry
  br label %omp.par.region1

omp.par.region1:                                  ; preds = %omp.par.region
  %omp_global_thread_num2 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @4)
  call void @__kmpc_barrier(%struct.ident_t* @3, i32 %omp_global_thread_num2)
  br label %omp.region.cont, !dbg !12

omp.region.cont:                                  ; preds = %omp.par.region1
  br label %omp.par.pre_finalize

omp.par.pre_finalize:                             ; preds = %omp.region.cont
  br label %omp.par.outlined.exit.exitStub
```


================
Comment at: openmp/runtime/test/ompt/cancel/cancel_parallel.c:1
-// RUN: %libomp-compile && env OMP_CANCELLATION=true %libomp-run | %sort-threads | FileCheck %s
+// RUN: %libomp-compile && env OMP_CANCELLATION=true %libomp-run \
+//      | %sort-threads | FileCheck %s
----------------
Nit: unrelated change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118409