[OpenMP][IRBuilder] Perform finalization (incl. outlining) late

[jdoerfert]

Summary

In order to fix PR44560 and to prepare for loop transformations we now
finalize a function late, which will also do the outlining late. The
logic is as before but the actual outlining step happens now after the
function was fully constructed. Once we have loop transformations we
can apply them in the finalize step before the outlining.

[rogfer01]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

114

This comment seems misplaced now.

659–660

I think you forgot to wrap these in a LLVM_DEBUG

666

This used to be called AfterIP. Probably the exact name is not that important but the unit tests still use AfterIP so I'd suggest to keep both names in sync.

[JonChesterfield]

Patch looks good with the above nits.

I'm not totally sure about the callback vs running a separate IR pass after the finalize() call, but when the callback is this simple it looks fine. I like that this preserves the current semantics.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

272

Calling getNewOutlineInfo will invalidate the references previously returned. That's not a bug in this patch but looks like it'll be easy to get wrong in future.

Perhaps the backing store should be a linked list, such that push_back doesn't invalidate any existing references?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

668

s/parallel/outlined?

[fghanim]

clang/lib/CodeGen/CodeGenFunction.cpp

113

Does this mean that finalize() is being called twice everytime, here and ~OpenMPIRBuilder()?
if yes, is there a reason why you want that?

[jdoerfert]

clang/lib/CodeGen/CodeGenFunction.cpp

113

It's called twice now, not that it matters. The basic rule is, call it at least once before you finalize the module. I'll remove the destructor for now to make it explicit only.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

272

The address of these objects is not stable but without that requirement we don't need a list. We make this function take a OutlineInfo object that we than move into the vector if that is preferred.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

114

Agreed, will be moved.

659–660

I simplify forgot the remove them. Thx for noticing.

666

I'll call it AfterIP again.

668

this is in the omp parallel callback so "parallel" is still fine.

[fghanim]

clang/lib/CodeGen/CodeGenFunction.cpp

113

Right now it doesn't matter, I agree. However, if later finalize() is modified for any reason, I worry this may introduce problems unintentionally.

In any case, if you decide to keep only one of them, I prefer to keep the one in ~OpenMPIRBuilder(). This way the OMPBuilder is self contained, so regardless of the frontend using it, we can be certain it's always called at the end. You can indicate explicitly in the description comment for finalize() that it is being called in the destructor, if you want.

[jdoerfert]

clang/lib/CodeGen/CodeGenFunction.cpp

113

That doesn't work, otherwise I would not call finalize here. At least it doesn't work without extra changes. As it is right now, the OMPBuilder in CodeGenModule is deleted *after* the IR is generated and emitted, so any changes at destructor time are too late.

[vsk]

Can this delete NeedWorkaroundForOpenMPIRBuilderBug from llvm/lib/Transforms/Utils/CodeExtractor.cpp?

[jdoerfert]

In D74372#1870547, @vsk wrote:

Can this delete NeedWorkaroundForOpenMPIRBuilderBug from llvm/lib/Transforms/Utils/CodeExtractor.cpp?

I didn't know about the workaround but that was the plan. I'll verify and add it to the patch.

[vsk]

Thanks :). IIRC check-clang is enough to exercise the relevant code path, as we get an assert in CodeExtractor without the workaround. (side note: please don't take my comments here as blocking, I just wanted to see if we could delete some cruft)

[fghanim]

clang/lib/CodeGen/CodeGenFunction.cpp

113

Got it. Then a minor suggestion is to just put a note in finalize() description about whatever you decide to do (i.e. keep both, remove the one in destructor, or something else entirely). This way, whoever is using/modifying the code is aware of it.

[jdoerfert]

Removed OMPIRBuilder workaround in extractor and addressed comments

[jdoerfert]

In D74372#1870620, @vsk wrote:

Thanks :). IIRC check-clang is enough to exercise the relevant code path, as we get an assert in CodeExtractor without the workaround. (side note: please don't take my comments here as blocking, I just wanted to see if we could delete some cruft)

Can you verify I deleted the workaround? I was not sure if the workaround is all of it or only the OMP... part.
I run the test you mentioned w/ and w/o the debug flag you mentioned in the PR, both times it passes w/o assertion:

PASS: Clang :: OpenMP/parallel_codegen.cpp

[jdoerfert]

Improve comment

[vsk]

llvm/lib/Transforms/Utils/CodeExtractor.cpp

1408

This part lgtm, thanks.

[jdoerfert]

I think this fixes the bug and allows loop optimizations later, let me know if there is anything else

[JonChesterfield]

Thanks!

[vvereschaka]

Hello @jdoerfert,

OpenMPIRBuilderTest.ParallelCancelBarrier is failed on Windows builders:

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-win-fast/builds/11242/steps/test-check-llvm-unit/logs/stdio
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/4537
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4534

C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\Frontend\OpenMPIRBuilderTest.cpp(612): error:       Expected: CI->getNextNode()->getSuccessor(0)

      Which is: 000000DC6479BEC0

To be equal to: ExitBB

      Which is: 000000DC6479B140

[  FAILED  ] OpenMPIRBuilderTest.ParallelCancelBarrier (4 ms)

Would you you fix the test or revert this commit?

[vvereschaka]

it also gets failed on the Linux builders

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/2843
http://lab.llvm.org:8011/builders/lld-x86_64-ubuntu-fast/builds/11581

[plotfi]

Could this be reverted for the time being so that bots can go green? There appear to be an awful lot of commits piling up on top of this one.

[jdoerfert]

In D74372#1873623, @plotfi wrote:

Could this be reverted for the time being so that bots can go green? There appear to be an awful lot of commits piling up on top of this one.

Should be fixed by now but you should revert if you feel it causes a problem.

[plotfi]

In D74372#1873665, @jdoerfert wrote:

In D74372#1873623, @plotfi wrote:

Could this be reverted for the time being so that bots can go green? There appear to be an awful lot of commits piling up on top of this one.

Should be fixed by now but you should revert if you feel it causes a problem.

Nice, thank you! https://github.com/llvm/llvm-project/commit/3f3ec9c40b2574929b41b93ac38484081b49837b did the trick.

clang/lib/CodeGen/CodeGenFunction.cpp

 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
+#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/FPEnv.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Transforms/Utils/PromoteMemToReg.h"
CodeGenFunction::~CodeGenFunction() {
   // If there are any unclaimed block infos, go ahead and destroy them
   // now.  This can happen if IR-gen gets clever and skips evaluating
   // something.
   if (FirstBlockInfo)
     destroyBlockInfos(FirstBlockInfo);
 
   if (getLangOpts().OpenMP && CurFn)
     CGM.getOpenMPRuntime().functionFinished(*this);
+
+  // If we have an OpenMPIRBuilder we want to finalize functions (incl.
+  // outlining etc) at some point. Doing it once the function codegen is done
+  // seems to be a reasonable spot. We do it here, as opposed to the deletion
+  // time of the CodeGenModule, because we have to ensure the IR has not yet
+  // been "emitted" to the outside, thus, modifications are still sensible.
+  if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder())
+    OMPBuilder->finalize();
 }
 
 // Map the LangOption for rounding mode into
 // the corresponding enum in the IR.
 static llvm::fp::RoundingMode ToConstrainedRoundingMD(
   LangOptions::FPRoundingModeKind Kind) {
 
   switch (Kind) {

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

public:
   /// not have an effect on \p M (see initialize).
   OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
 
   /// Initialize the internal state, this will put structures types and
   /// potentially other helpers into the underlying module. Must be called
   /// before any other method and only once!
   void initialize();
 
+  /// Finalize the underlying module, e.g., by outlining regions.
+  void finalize();
+
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function &Fn);
 
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
   /// Callback type for variable finalization (think destructors).
   ///
private:
   /// The LLVM-IR Builder used to create IR.
   IRBuilder<> Builder;
 
   /// Map to remember source location strings
   StringMap<Constant *> SrcLocStrMap;
 
   /// Map to remember existing ident_t*.
   DenseMap<std::pair<Constant *, uint64_t>, GlobalVariable *> IdentMap;
+
+  /// Helper that contains information about regions we need to outline
+  /// during finalization.
+  struct OutlineInfo {
+    SmallVector<BasicBlock *, 32> Blocks;
+    using PostOutlineCBTy = std::function<void(Function &)>;
+    PostOutlineCBTy PostOutlineCB;
+  };
+
+  /// Collection of regions that need to be outlined during finalization.
+  SmallVector<OutlineInfo, 16> OutlineInfos;
+
+  /// Add a new region that will be outlined later.
+  void addOutlineInfo(OutlineInfo &&OI) { OutlineInfos.emplace_back(OI); }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_IR_IRBUILDER_H

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

#include "llvm/Frontend/OpenMP/OMPKinds.def"
   }
 
   assert(Fn && "Failed to create OpenMP runtime function");
   return Fn;
 }
 
 void OpenMPIRBuilder::initialize() { initializeTypes(M); }
 
+void OpenMPIRBuilder::finalize() {
+  for (OutlineInfo &OI : OutlineInfos) {
+    assert(!OI.Blocks.empty() &&
+           "Outlined regions should have at least a single block!");
+    BasicBlock *RegEntryBB = OI.Blocks.front();
+    Function *OuterFn = RegEntryBB->getParent();
+    CodeExtractorAnalysisCache CEAC(*OuterFn);
+    CodeExtractor Extractor(OI.Blocks, /* DominatorTree */ nullptr,
+                            /* AggregateArgs */ false,
+                            /* BlockFrequencyInfo */ nullptr,
+                            /* BranchProbabilityInfo */ nullptr,
+                            /* AssumptionCache */ nullptr,
+                            /* AllowVarArgs */ true,
+                            /* AllowAlloca */ true,
+                            /* Suffix */ ".omp_par");
+
+    LLVM_DEBUG(dbgs() << "Before     outlining: " << *OuterFn << "\n");
+
+    Function *OutlinedFn = Extractor.extractCodeRegion(CEAC);
+
+    LLVM_DEBUG(dbgs() << "After      outlining: " << *OuterFn << "\n");
+    LLVM_DEBUG(dbgs() << "   Outlined function: " << *OutlinedFn << "\n");
+
+    // For compability with the clang CG we move the outlined function after the
+    // one with the parallel region.
+    OutlinedFn->removeFromParent();
+    M.getFunctionList().insertAfter(OuterFn->getIterator(), OutlinedFn);
+
+    // Remove the artificial entry introduced by the extractor right away, we
+    // made our own entry block after all.
+    {
+      BasicBlock &ArtificialEntry = OutlinedFn->getEntryBlock();
+      assert(ArtificialEntry.getUniqueSuccessor() == RegEntryBB);
+      assert(RegEntryBB->getUniquePredecessor() == &ArtificialEntry);
+      RegEntryBB->moveBefore(&ArtificialEntry);
+      ArtificialEntry.eraseFromParent();
+    }
+    assert(&OutlinedFn->getEntryBlock() == RegEntryBB);
+    assert(OutlinedFn && OutlinedFn->getNumUses() == 1);
+
+    // Run a user callback, e.g. to add attributes.
+    if (OI.PostOutlineCB)
+      OI.PostOutlineCB(*OutlinedFn);
+  }
+
+  // Allow finalize to be called multiple times.
+  OutlineInfos.clear();
+}
+
 Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
                                          IdentFlag LocFlags) {
   // Enable "C-mode".
   LocFlags |= OMP_IDENT_FLAG_KMPC;
 
   GlobalVariable *&DefaultIdent = IdentMap[{SrcLocStr, uint64_t(LocFlags)}];
   if (!DefaultIdent) {
     Constant *I32Null = ConstantInt::getNullValue(Int32);
IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
   //   |
   //   V
   // PRegPreFiniBB          <- The block we will start finalization from.
   //   |
   //   V
   // PRegionExitBB          <- A common exit to simplify block collection.
   //
 
-  LLVM_DEBUG(dbgs() << "Before body codegen: " << *UI->getFunction() << "\n");
+  LLVM_DEBUG(dbgs() << "Before body codegen: " << *OuterFn << "\n");
 
   // Let the caller create the body.
   assert(BodyGenCB && "Expected body generation callback!");
   InsertPointTy CodeGenIP(PRegBodyBB, PRegBodyBB->begin());
   BodyGenCB(AllocaIP, CodeGenIP, *PRegPreFiniBB);
 
-  LLVM_DEBUG(dbgs() << "After  body codegen: " << *UI->getFunction() << "\n");
+  LLVM_DEBUG(dbgs() << "After  body codegen: " << *OuterFn << "\n");
 
+  OutlineInfo OI;
   SmallPtrSet<BasicBlock *, 32> ParallelRegionBlockSet;
-  SmallVector<BasicBlock *, 32> ParallelRegionBlocks, Worklist;
+  SmallVector<BasicBlock *, 32> Worklist;
   ParallelRegionBlockSet.insert(PRegEntryBB);
   ParallelRegionBlockSet.insert(PRegExitBB);
 
   // Collect all blocks in-between PRegEntryBB and PRegExitBB.
   Worklist.push_back(PRegEntryBB);
   while (!Worklist.empty()) {
     BasicBlock *BB = Worklist.pop_back_val();
-    ParallelRegionBlocks.push_back(BB);
+    OI.Blocks.push_back(BB);
     for (BasicBlock *SuccBB : successors(BB))
       if (ParallelRegionBlockSet.insert(SuccBB).second)
         Worklist.push_back(SuccBB);
   }
 
   CodeExtractorAnalysisCache CEAC(*OuterFn);
-  CodeExtractor Extractor(ParallelRegionBlocks, /* DominatorTree */ nullptr,
+  CodeExtractor Extractor(OI.Blocks, /* DominatorTree */ nullptr,
                           /* AggregateArgs */ false,
                           /* BlockFrequencyInfo */ nullptr,
                           /* BranchProbabilityInfo */ nullptr,
                           /* AssumptionCache */ nullptr,
                           /* AllowVarArgs */ true,
                           /* AllowAlloca */ true,
                           /* Suffix */ ".omp_par");
 
   // Find inputs to, outputs from the code region.
   BasicBlock *CommonExit = nullptr;
   SetVector<Value *> Inputs, Outputs, SinkingCands, HoistingCands;
   Extractor.findAllocas(CEAC, SinkingCands, HoistingCands, CommonExit);
   Extractor.findInputsOutputs(Inputs, Outputs, SinkingCands);
 
-  LLVM_DEBUG(dbgs() << "Before privatization: " << *UI->getFunction() << "\n");
+  LLVM_DEBUG(dbgs() << "Before privatization: " << *OuterFn << "\n");
 
   FunctionCallee TIDRTLFn =
       getOrCreateRuntimeFunction(OMPRTL___kmpc_global_thread_num);
 
   auto PrivHelper = [&](Value &V) {
     if (&V == TIDAddr || &V == ZeroAddr)
       return;
 
for (Value *Input : Inputs) {
     LLVM_DEBUG(dbgs() << "Captured input: " << *Input << "\n");
     PrivHelper(*Input);
   }
   for (Value *Output : Outputs) {
     LLVM_DEBUG(dbgs() << "Captured output: " << *Output << "\n");
     PrivHelper(*Output);
   }
 
-  LLVM_DEBUG(dbgs() << "After  privatization: " << *UI->getFunction() << "\n");
+  LLVM_DEBUG(dbgs() << "After  privatization: " << *OuterFn << "\n");
   LLVM_DEBUG({
-    for (auto *BB : ParallelRegionBlocks)
+    for (auto *BB : OI.Blocks)
       dbgs() << " PBR: " << BB->getName() << "\n";
   });
 
-  // Add some known attributes to the outlined function.
-  Function *OutlinedFn = Extractor.extractCodeRegion(CEAC);
-  OutlinedFn->addParamAttr(0, Attribute::NoAlias);
-  OutlinedFn->addParamAttr(1, Attribute::NoAlias);
-  OutlinedFn->addFnAttr(Attribute::NoUnwind);
-  OutlinedFn->addFnAttr(Attribute::NoRecurse);
-
-  LLVM_DEBUG(dbgs() << "After      outlining: " << *UI->getFunction() << "\n");
-  LLVM_DEBUG(dbgs() << "   Outlined function: " << *OutlinedFn << "\n");
-
-  // For compability with the clang CG we move the outlined function after the
-  // one with the parallel region.
-  OutlinedFn->removeFromParent();
-  M.getFunctionList().insertAfter(OuterFn->getIterator(), OutlinedFn);
-
-  // Remove the artificial entry introduced by the extractor right away, we
-  // made our own entry block after all.
-  {
-    BasicBlock &ArtificialEntry = OutlinedFn->getEntryBlock();
-    assert(ArtificialEntry.getUniqueSuccessor() == PRegEntryBB);
-    assert(PRegEntryBB->getUniquePredecessor() == &ArtificialEntry);
-    PRegEntryBB->moveBefore(&ArtificialEntry);
-    ArtificialEntry.eraseFromParent();
-  }
-  LLVM_DEBUG(dbgs() << "PP Outlined function: " << *OutlinedFn << "\n");
-  assert(&OutlinedFn->getEntryBlock() == PRegEntryBB);
-
-  assert(OutlinedFn && OutlinedFn->getNumUses() == 1);
-  assert(OutlinedFn->arg_size() >= 2 &&
-         "Expected at least tid and bounded tid as arguments");
-  unsigned NumCapturedVars = OutlinedFn->arg_size() - /* tid & bounded tid */ 2;
-
-  CallInst *CI = cast<CallInst>(OutlinedFn->user_back());
-  CI->getParent()->setName("omp_parallel");
-  Builder.SetInsertPoint(CI);
-
-  // Build call __kmpc_fork_call(Ident, n, microtask, var1, .., varn);
-  Value *ForkCallArgs[] = {Ident, Builder.getInt32(NumCapturedVars),
-                           Builder.CreateBitCast(OutlinedFn, ParallelTaskPtr)};
-
-  SmallVector<Value *, 16> RealArgs;
-  RealArgs.append(std::begin(ForkCallArgs), std::end(ForkCallArgs));
-  RealArgs.append(CI->arg_begin() + /* tid & bound tid */ 2, CI->arg_end());
-
   FunctionCallee RTLFn = getOrCreateRuntimeFunction(OMPRTL___kmpc_fork_call);
   if (auto *F = dyn_cast<llvm::Function>(RTLFn.getCallee())) {
     if (!F->hasMetadata(llvm::LLVMContext::MD_callback)) {
       llvm::LLVMContext &Ctx = F->getContext();
       MDBuilder MDB(Ctx);
       // Annotate the callback behavior of the __kmpc_fork_call:
       //  - The callback callee is argument number 2 (microtask).
       //  - The first two arguments of the callback callee are unknown (-1).
       //  - All variadic arguments to the __kmpc_fork_call are passed to the
       //    callback callee.
       F->addMetadata(
           llvm::LLVMContext::MD_callback,
-          *llvm::MDNode::get(Ctx, {MDB.createCallbackEncoding(
-                                      2, {-1, -1},
+          *llvm::MDNode::get(
+              Ctx, {MDB.createCallbackEncoding(2, {-1, -1},
                                                /* VarArgsArePassed */ true)}));
     }
   }
 
+  OI.PostOutlineCB = [=](Function &OutlinedFn) {
+    // Add some known attributes.
+    OutlinedFn.addParamAttr(0, Attribute::NoAlias);
+    OutlinedFn.addParamAttr(1, Attribute::NoAlias);
+    OutlinedFn.addFnAttr(Attribute::NoUnwind);
+    OutlinedFn.addFnAttr(Attribute::NoRecurse);
+
+    assert(OutlinedFn.arg_size() >= 2 &&
+           "Expected at least tid and bounded tid as arguments");
+    unsigned NumCapturedVars =
+        OutlinedFn.arg_size() - /* tid & bounded tid */ 2;
+
+    CallInst *CI = cast<CallInst>(OutlinedFn.user_back());
+    CI->getParent()->setName("omp_parallel");
+    Builder.SetInsertPoint(CI);
+
+    // Build call __kmpc_fork_call(Ident, n, microtask, var1, .., varn);
+    Value *ForkCallArgs[] = {
+        Ident, Builder.getInt32(NumCapturedVars),
+        Builder.CreateBitCast(&OutlinedFn, ParallelTaskPtr)};
+
+    SmallVector<Value *, 16> RealArgs;
+    RealArgs.append(std::begin(ForkCallArgs), std::end(ForkCallArgs));
+    RealArgs.append(CI->arg_begin() + /* tid & bound tid */ 2, CI->arg_end());
+
     Builder.CreateCall(RTLFn, RealArgs);
 
     LLVM_DEBUG(dbgs() << "With fork_call placed: "
                       << *Builder.GetInsertBlock()->getParent() << "\n");
 
-  InsertPointTy AfterIP(UI->getParent(), UI->getParent()->end());
     InsertPointTy ExitIP(PRegExitBB, PRegExitBB->end());
-  UI->eraseFromParent();
 
     // Initialize the local TID stack location with the argument value.
     Builder.SetInsertPoint(PrivTID);
-  Function::arg_iterator OutlinedAI = OutlinedFn->arg_begin();
+    Function::arg_iterator OutlinedAI = OutlinedFn.arg_begin();
     Builder.CreateStore(Builder.CreateLoad(OutlinedAI), PrivTIDAddr);
 
     // If no "if" clause was present we do not need the call created during
     // outlining, otherwise we reuse it in the serialized parallel region.
     if (!ElseTI) {
       CI->eraseFromParent();
     } else {
 
       // If an "if" clause was present we are now generating the serialized
       // version into the "else" branch.
       Builder.SetInsertPoint(ElseTI);
 
       // Build calls __kmpc_serialized_parallel(&Ident, GTid);
       Value *SerializedParallelCallArgs[] = {Ident, ThreadID};
       Builder.CreateCall(
           getOrCreateRuntimeFunction(OMPRTL___kmpc_serialized_parallel),
           SerializedParallelCallArgs);
 
       // OutlinedFn(&GTid, &zero, CapturedStruct);
       CI->removeFromParent();
       Builder.Insert(CI);
 
       // __kmpc_end_serialized_parallel(&Ident, GTid);
       Value *EndArgs[] = {Ident, ThreadID};
       Builder.CreateCall(
           getOrCreateRuntimeFunction(OMPRTL___kmpc_end_serialized_parallel),
           EndArgs);
 
       LLVM_DEBUG(dbgs() << "With serialized parallel region: "
                         << *Builder.GetInsertBlock()->getParent() << "\n");
     }
 
+    for (Instruction *I : ToBeDeleted)
+      I->eraseFromParent();
+  };
+
   // Adjust the finalization stack, verify the adjustment, and call the
-  // finalize function a last time to finalize values between the pre-fini block
-  // and the exit block if we left the parallel "the normal way".
+  // finalize function a last time to finalize values between the pre-fini
+  // block and the exit block if we left the parallel "the normal way".
   auto FiniInfo = FinalizationStack.pop_back_val();
   (void)FiniInfo;
   assert(FiniInfo.DK == OMPD_parallel &&
          "Unexpected finalization stack state!");
 
   Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
   assert(PreFiniTI->getNumSuccessors() == 1 &&
-         PreFiniTI->getSuccessor(0)->size() == 1 &&
-         isa<ReturnInst>(PreFiniTI->getSuccessor(0)->getTerminator()) &&
+         PreFiniTI->getSuccessor(0) == PRegExitBB &&
          "Unexpected CFG structure!");
 
   InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
   FiniCB(PreFiniIP);
 
-  for (Instruction *I : ToBeDeleted)
-    I->eraseFromParent();
+  InsertPointTy AfterIP(UI->getParent(), UI->getParent()->end());
+  UI->eraseFromParent();
+
+  // Register the outlined info.
+  addOutlineInfo(std::move(OI));
 
   return AfterIP;
 }
 
 void OpenMPIRBuilder::emitFlush(const LocationDescription &Loc)
 {
   // Build call void __kmpc_flush(ident_t *loc)
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
   Value *Args[] = {getOrCreateIdent(SrcLocStr)};
 
   Builder.CreateCall(getOrCreateRuntimeFunction(OMPRTL___kmpc_flush), Args);
 }
 
 void OpenMPIRBuilder::CreateFlush(const LocationDescription &Loc)
 {
   if (!updateToLocation(Loc))
    return;
   emitFlush(Loc);
 }

llvm/lib/Transforms/Utils/CodeExtractor.cpp

 /// Fix up the debug info in the old and new functions by pointing line
 /// locations and debug intrinsics to the new subprogram scope, and by deleting
 /// intrinsics which point to values outside of the new function.
 static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
                                          CallInst &TheCall) {
   DISubprogram *OldSP = OldFunc.getSubprogram();
   LLVMContext &Ctx = OldFunc.getContext();
 
-  // See llvm.org/PR44560, OpenMP passes an invalid subprogram to CodeExtractor.
-  bool NeedWorkaroundForOpenMPIRBuilderBug =
-      OldSP && OldSP->getRetainedNodes()->isTemporary();
-
-  if (!OldSP || NeedWorkaroundForOpenMPIRBuilderBug) {
+  if (!OldSP) {
     // Erase any debug info the new function contains.
     stripDebugInfo(NewFunc);
     // Make sure the old function doesn't contain any non-local metadata refs.
     eraseDebugIntrinsicsWithNonLocalRefs(NewFunc);
     return;
   }
 
   // Create a subprogram for the new function. Leave out a description of the

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
   EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
   EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
   EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
   EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
 
   EXPECT_EQ(cast<CallInst>(Barrier)->getArgOperand(1), GTID);
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_FALSE(verifyModule(*M, &errs()));
 }
 
 TEST_F(OpenMPIRBuilderTest, CreateCancel) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
TEST_F(OpenMPIRBuilderTest, CreateCancel) {
   EXPECT_EQ(CancelBBTI->getSuccessor(1)->getTerminator()->getSuccessor(0),
             CBB);
 
   EXPECT_EQ(cast<CallInst>(Cancel)->getArgOperand(1), GTID);
 
   OMPBuilder.popFinalizationCB();
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_FALSE(verifyModule(*M, &errs()));
 }
 
 TEST_F(OpenMPIRBuilderTest, CreateCancelIfCond) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
TEST_F(OpenMPIRBuilderTest, CreateCancelIfCond) {
   EXPECT_EQ(CancelBBTI->getSuccessor(1)->getTerminator()->getSuccessor(0),
             CBB);
 
   EXPECT_EQ(cast<CallInst>(Cancel)->getArgOperand(1), GTID);
 
   OMPBuilder.popFinalizationCB();
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_FALSE(verifyModule(*M, &errs()));
 }
 
 TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
   EXPECT_EQ(BarrierBBTI->getSuccessor(1)->getTerminator()->getSuccessor(0),
             CBB);
 
   EXPECT_EQ(cast<CallInst>(Barrier)->getArgOperand(1), GTID);
 
   OMPBuilder.popFinalizationCB();
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_FALSE(verifyModule(*M, &errs()));
 }
 
 TEST_F(OpenMPIRBuilderTest, DbgLoc) {
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
   F->setName("func");
 
   IRBuilder<> Builder(BB);
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
     ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
     Builder.restoreIP(CodeGenIP);
     Builder.CreateStore(V, ReplacementValue);
     return CodeGenIP;
   };
 
   auto FiniCB = [&](InsertPointTy CodeGenIP) { ++NumFinalizationPoints; };
 
-  IRBuilder<>::InsertPoint AfterIP = OMPBuilder.CreateParallel(
-      Loc, BodyGenCB, PrivCB, FiniCB, nullptr, nullptr, OMP_PROC_BIND_default, false);
-
+  IRBuilder<>::InsertPoint AfterIP =
+      OMPBuilder.CreateParallel(Loc, BodyGenCB, PrivCB, FiniCB, nullptr,
+                                nullptr, OMP_PROC_BIND_default, false);
   EXPECT_EQ(NumBodiesGenerated, 1U);
   EXPECT_EQ(NumPrivatizedVars, 1U);
   EXPECT_EQ(NumFinalizationPoints, 1U);
 
   Builder.restoreIP(AfterIP);
   Builder.CreateRetVoid();
 
+  OMPBuilder.finalize();
+
   EXPECT_NE(PrivAI, nullptr);
   Function *OutlinedFn = PrivAI->getFunction();
   EXPECT_NE(F, OutlinedFn);
-  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_FALSE(verifyModule(*M, &errs()));
   EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoUnwind));
   EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoRecurse));
   EXPECT_TRUE(OutlinedFn->hasParamAttribute(0, Attribute::NoAlias));
   EXPECT_TRUE(OutlinedFn->hasParamAttribute(1, Attribute::NoAlias));
 
   EXPECT_TRUE(OutlinedFn->hasInternalLinkage());
   EXPECT_EQ(OutlinedFn->arg_size(), 3U);
 
IRBuilder<>::InsertPoint AfterIP = OMPBuilder.CreateParallel(
       nullptr, OMP_PROC_BIND_default, false);
 
   EXPECT_EQ(NumBodiesGenerated, 1U);
   EXPECT_EQ(NumPrivatizedVars, 1U);
   EXPECT_EQ(NumFinalizationPoints, 1U);
 
   Builder.restoreIP(AfterIP);
   Builder.CreateRetVoid();
+  OMPBuilder.finalize();
 
   EXPECT_NE(PrivAI, nullptr);
   Function *OutlinedFn = PrivAI->getFunction();
   EXPECT_NE(F, OutlinedFn);
   EXPECT_FALSE(verifyModule(*M, &errs()));
 
   EXPECT_TRUE(OutlinedFn->hasInternalLinkage());
   EXPECT_EQ(OutlinedFn->arg_size(), 3U);
TEST_F(OpenMPIRBuilderTest, ParallelCancelBarrier) {
 
   EXPECT_EQ(NumBodiesGenerated, 1U);
   EXPECT_EQ(NumPrivatizedVars, 0U);
   EXPECT_EQ(NumFinalizationPoints, 2U);
   EXPECT_EQ(FakeDestructor->getNumUses(), 2U);
 
   Builder.restoreIP(AfterIP);
   Builder.CreateRetVoid();
+  OMPBuilder.finalize();
 
   EXPECT_FALSE(verifyModule(*M, &errs()));
 
   BasicBlock *ExitBB = nullptr;
   for (const User *Usr : FakeDestructor->users()) {
     const CallInst *CI = dyn_cast<CallInst>(Usr);
     ASSERT_EQ(CI->getCalledFunction(), FakeDestructor);
     ASSERT_TRUE(isa<BranchInst>(CI->getNextNode()));