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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
