This is an archive of the discontinued LLVM Phabricator instance.

[OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.
ClosedPublic

Authored by Meinersbur on Jan 27 2022, 1:40 PM.

Details

Summary

The callback is expected to create a branch to the ContinuationBB (sometimes called FiniBB in some lambdas) argument when finishing. This creates problems:

  1. The InsertPoint used for CodeGenIP does not need to be the end of a block. If it is not, a naive callback will insert a branch instruction into the middle of the block.
  2. The BasicBlock the CodeGenIP is pointing to may or may not have a terminator. There is an conflict where to branch to if the block already has a terminator.
  3. Some API functions work only with block having a terminator. Some workarounds have been used to insert a temporary terminator that is removed again.
  4. Some callbacks are sensitive to whether the BasicBlock has a terminator or not. This creates a callback ordering problem where different callback may have different behaviour depending on whether a previous callback created a terminator or not. The problem also exists for FinalizeCallbackTy where some callbacks do create branch to another "continue" block, but unlike BodyGenCallbackTy does not receive the target as argument. This is not addressed in this patch.

With this patch, the callback receives an CodeGenIP into a BasicBlock where to insert instructions. If it has to insert control flow, it can split the block at that position as needed but otherwise no separate ContinuationBB is needed. In particular, a callback can be empty without breaking the emitted IR. If the caller needs the control flow to branch to a specific target, it can insert the branch instruction itself and pass an InsertPoint before the terminator to the callback.

Certain frontends such as Clang may expect the current IRBuilder position to be at the end of a basic block. In this case its callbacks must split the block at CodeGenIP before setting the IRBuilder position such that the instructions after CodeGenIP are moved to another basic block and before returning create a new branch instruction to the split block.

Some utility functions such as splitBB are supporting correct splitting of BasicBlocks, independent of whether they have a terminator or not, returning/setting the InsertPoint of an IRBuilder to the end of split predecessor block, and optionally omitting creating a branch to the split successor block to be added later.

Diff Detail

Event Timeline

Meinersbur created this revision.Jan 27 2022, 1:40 PM
Meinersbur requested review of this revision.Jan 27 2022, 1:40 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJan 27 2022, 1:40 PM
ormris removed a subscriber: ormris.Jan 27 2022, 3:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 3:39 PM

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).

clang/lib/CodeGen/CGStmtOpenMP.cpp
5603–5605

Why is it not possible to use OMPBuilderCBHelpers::EmitOMPInlinedRegionBody here?

clang/lib/CodeGen/CodeGenFunction.h
1825

Nit:finalize

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
26–58

I guess these are already in from your other patch (https://reviews.llvm.org/D114413).

147

Nit:itself

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1301–1307

Nit:Unexpected

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
127

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
openmp/runtime/test/ompt/cancel/cancel_parallel.c
1 ↗(On Diff #403787)

Nit: unrelated change?

Meinersbur marked 6 inline comments as done.
  • Rebase
  • Address review
clang/lib/CodeGen/CGStmtOpenMP.cpp
5603–5605

EmitOMPInlinedRegionBody calls splitBBWithSuffix, which has already been done in this lambda.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
26–58

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!!).

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
127

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.

LGTM. You may wait for a day in case there are comments from other reviewers.

I think the CI is complaining about some clang-format issue in clang/test/OpenMP/critical_codegen_attr.cpp

This revision is now accepted and ready to land.Apr 26 2022, 2:53 AM
Meinersbur added a comment.EditedApr 26 2022, 9:24 AM

I think the CI is complaining about some clang-format issue in clang/test/OpenMP/critical_codegen_attr.cpp

clang-format should not be applied on Clang tests, many of them have significant whitespace (eg. the preprocessor tests). In this case, clang-suggests to indent the // CHECK: lines by 2 spaces (to match the indentation of the statements), which seems unnecessary.

This revision was landed with ongoing or failed builds.Apr 26 2022, 2:35 PM
This revision was automatically updated to reflect the committed changes.
jdoerfert added inline comments.Jul 6 2022, 11:56 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
70

Argh... Why do we need to copy llvm functionality instead of making it work for our use case?
Nit: no llvm::.

clang/test/OpenMP/master_codegen.cpp