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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.


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


================
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
----------------
kiranchandramohan wrote:
> I guess these are already in from your other patch (https://reviews.llvm.org/D114413).
Yes, now public because also used in CodegenOpenMP.

These utility functions avoid all the workarounds that `splitBasicBlock`/`SplitBlock` do not all degenerate blocks, such as inserting temporary terminators or ContinuationBB (There may be insertion points referencing those temporary terminators!!).


================
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();
----------------
kiranchandramohan wrote:
> 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
> ```
Empty BBs should not be considered an issue since they are removed by SimplifyCFG anyway. Why would it be? Correctness should be the priority. 

What makes the current code so fragile are the conditions that depend on the current insert point. Invoking it in a different manner causes an untested code path to be taken. For instance, an insert point becomes invalided that did not under any previous test because nobody yet inserted new BBs in a callback. One if condition inside the callback and everything breaks.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118409