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