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

This reverts commit 8a56d64d7620b3764f10f03f3a1e307fcdd72c2f.

Will be recommitted once the clang test problem is addressed.

include/llvm/Frontend/OpenMP/OMPIRBuilder.h

@@ -34,9 +34,6 @@ public:
   /// 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);
 
@@ -257,20 +254,6 @@ private:
 
   /// 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

lib/Frontend/OpenMP/OMPIRBuilder.cpp

@@ -93,55 +93,6 @@ Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
 
 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".
@@ -464,18 +415,17 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
   // PRegionExitBB          <- A common exit to simplify block collection.
   //
 
-  LLVM_DEBUG(dbgs() << "Before body codegen: " << *OuterFn << "\n");
+  LLVM_DEBUG(dbgs() << "Before body codegen: " << *UI->getFunction() << "\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: " << *OuterFn << "\n");
+  LLVM_DEBUG(dbgs() << "After  body codegen: " << *UI->getFunction() << "\n");
 
-  OutlineInfo OI;
   SmallPtrSet<BasicBlock *, 32> ParallelRegionBlockSet;
-  SmallVector<BasicBlock *, 32> Worklist;
+  SmallVector<BasicBlock *, 32> ParallelRegionBlocks, Worklist;
   ParallelRegionBlockSet.insert(PRegEntryBB);
   ParallelRegionBlockSet.insert(PRegExitBB);
 
@@ -483,14 +433,14 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
   Worklist.push_back(PRegEntryBB);
   while (!Worklist.empty()) {
     BasicBlock *BB = Worklist.pop_back_val();
-    OI.Blocks.push_back(BB);
+    ParallelRegionBlocks.push_back(BB);
     for (BasicBlock *SuccBB : successors(BB))
       if (ParallelRegionBlockSet.insert(SuccBB).second)
         Worklist.push_back(SuccBB);
   }
 
   CodeExtractorAnalysisCache CEAC(*OuterFn);
-  CodeExtractor Extractor(OI.Blocks, /* DominatorTree */ nullptr,
+  CodeExtractor Extractor(ParallelRegionBlocks, /* DominatorTree */ nullptr,
                           /* AggregateArgs */ false,
                           /* BlockFrequencyInfo */ nullptr,
                           /* BranchProbabilityInfo */ nullptr,
@@ -505,7 +455,7 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
   Extractor.findAllocas(CEAC, SinkingCands, HoistingCands, CommonExit);
   Extractor.findInputsOutputs(Inputs, Outputs, SinkingCands);
 
-  LLVM_DEBUG(dbgs() << "Before privatization: " << *OuterFn << "\n");
+  LLVM_DEBUG(dbgs() << "Before privatization: " << *UI->getFunction() << "\n");
 
   FunctionCallee TIDRTLFn =
       getOrCreateRuntimeFunction(OMPRTL___kmpc_global_thread_num);
@@ -546,12 +496,56 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
     PrivHelper(*Output);
   }
 
-  LLVM_DEBUG(dbgs() << "After  privatization: " << *OuterFn << "\n");
+  LLVM_DEBUG(dbgs() << "After  privatization: " << *UI->getFunction() << "\n");
   LLVM_DEBUG({
-    for (auto *BB : OI.Blocks)
+    for (auto *BB : ParallelRegionBlocks)
       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)) {
@@ -564,86 +558,59 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
       //    callback callee.
       F->addMetadata(
           llvm::LLVMContext::MD_callback,
-          *llvm::MDNode::get(
+          *llvm::MDNode::get(Ctx, {MDB.createCallbackEncoding(
-              Ctx, {MDB.createCallbackEncoding(2, {-1, -1},
+                                      2, {-1, -1},
-                                               /* VarArgsArePassed */ true)}));
+                                      /* VarArgsArePassed */ true)}));
     }
   }
 
-  OI.PostOutlineCB = [=](Function &OutlinedFn) {
+  Builder.CreateCall(RTLFn, RealArgs);
-    // 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 &&
+  LLVM_DEBUG(dbgs() << "With fork_call placed: "
-           "Expected at least tid and bounded tid as arguments");
+                    << *Builder.GetInsertBlock()->getParent() << "\n");
-    unsigned NumCapturedVars =
-        OutlinedFn.arg_size() - /* tid & bounded tid */ 2;
 
-    CallInst *CI = cast<CallInst>(OutlinedFn.user_back());
+  InsertPointTy AfterIP(UI->getParent(), UI->getParent()->end());
-    CI->getParent()->setName("omp_parallel");
+  InsertPointTy ExitIP(PRegExitBB, PRegExitBB->end());
-    Builder.SetInsertPoint(CI);
+  UI->eraseFromParent();
 
-    // Build call __kmpc_fork_call(Ident, n, microtask, var1, .., varn);
+  // Initialize the local TID stack location with the argument value.
-    Value *ForkCallArgs[] = {
+  Builder.SetInsertPoint(PrivTID);
-        Ident, Builder.getInt32(NumCapturedVars),
+  Function::arg_iterator OutlinedAI = OutlinedFn->arg_begin();
-        Builder.CreateBitCast(&OutlinedFn, ParallelTaskPtr)};
+  Builder.CreateStore(Builder.CreateLoad(OutlinedAI), PrivTIDAddr);
 
-    SmallVector<Value *, 16> RealArgs;
+  // If no "if" clause was present we do not need the call created during
-    RealArgs.append(std::begin(ForkCallArgs), std::end(ForkCallArgs));
+  // outlining, otherwise we reuse it in the serialized parallel region.
-    RealArgs.append(CI->arg_begin() + /* tid & bound tid */ 2, CI->arg_end());
+  if (!ElseTI) {
+    CI->eraseFromParent();
+  } else {
 
-    Builder.CreateCall(RTLFn, RealArgs);
+    // If an "if" clause was present we are now generating the serialized
+    // version into the "else" branch.
+    Builder.SetInsertPoint(ElseTI);
 
-    LLVM_DEBUG(dbgs() << "With fork_call placed: "
+    // 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");
-
+  }
-    InsertPointTy ExitIP(PRegExitBB, PRegExitBB->end());
-
-    // Initialize the local TID stack location with the argument value.
-    Builder.SetInsertPoint(PrivTID);
-    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
+  // finalize function a last time to finalize values between the pre-fini block
-  // block and the exit block if we left the parallel "the normal way".
+  // 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 &&
@@ -651,17 +618,15 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
 
   Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
   assert(PreFiniTI->getNumSuccessors() == 1 &&
-         PreFiniTI->getSuccessor(0) == PRegExitBB &&
+         PreFiniTI->getSuccessor(0)->size() == 1 &&
+         isa<ReturnInst>(PreFiniTI->getSuccessor(0)->getTerminator()) &&
          "Unexpected CFG structure!");
 
   InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
   FiniCB(PreFiniIP);
 
-  InsertPointTy AfterIP(UI->getParent(), UI->getParent()->end());
+  for (Instruction *I : ToBeDeleted)
-  UI->eraseFromParent();
+    I->eraseFromParent();
-
-  // Register the outlined info.
-  addOutlineInfo(std::move(OI));
 
   return AfterIP;
 }

lib/Transforms/Utils/CodeExtractor.cpp

@@ -1405,7 +1405,11 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
   DISubprogram *OldSP = OldFunc.getSubprogram();
   LLVMContext &Ctx = OldFunc.getContext();
 
-  if (!OldSP) {
+  // See llvm.org/PR44560, OpenMP passes an invalid subprogram to CodeExtractor.
+  bool NeedWorkaroundForOpenMPIRBuilderBug =
+      OldSP && OldSP->getRetainedNodes()->isTemporary();
+
+  if (!OldSP || NeedWorkaroundForOpenMPIRBuilderBug) {
     // Erase any debug info the new function contains.
     stripDebugInfo(NewFunc);
     // Make sure the old function doesn't contain any non-local metadata refs.

unittests/Frontend/OpenMPIRBuilderTest.cpp

@@ -96,7 +96,7 @@ TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
   EXPECT_EQ(cast<CallInst>(Barrier)->getArgOperand(1), GTID);
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M, &errs()));
+  EXPECT_FALSE(verifyModule(*M));
 }
 
 TEST_F(OpenMPIRBuilderTest, CreateCancel) {
@@ -151,7 +151,7 @@ TEST_F(OpenMPIRBuilderTest, CreateCancel) {
   OMPBuilder.popFinalizationCB();
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M, &errs()));
+  EXPECT_FALSE(verifyModule(*M));
 }
 
 TEST_F(OpenMPIRBuilderTest, CreateCancelIfCond) {
@@ -212,7 +212,7 @@ TEST_F(OpenMPIRBuilderTest, CreateCancelIfCond) {
   OMPBuilder.popFinalizationCB();
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M, &errs()));
+  EXPECT_FALSE(verifyModule(*M));
 }
 
 TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
@@ -267,7 +267,7 @@ TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
   OMPBuilder.popFinalizationCB();
 
   Builder.CreateUnreachable();
-  EXPECT_FALSE(verifyModule(*M, &errs()));
+  EXPECT_FALSE(verifyModule(*M));
 }
 
 TEST_F(OpenMPIRBuilderTest, DbgLoc) {
@@ -362,9 +362,9 @@ TEST_F(OpenMPIRBuilderTest, ParallelSimple) {
 
   auto FiniCB = [&](InsertPointTy CodeGenIP) { ++NumFinalizationPoints; };
 
-  IRBuilder<>::InsertPoint AfterIP =
+  IRBuilder<>::InsertPoint AfterIP = OMPBuilder.CreateParallel(
-      OMPBuilder.CreateParallel(Loc, BodyGenCB, PrivCB, FiniCB, nullptr,
+      Loc, BodyGenCB, PrivCB, FiniCB, nullptr, nullptr, OMP_PROC_BIND_default, false);
-                                nullptr, OMP_PROC_BIND_default, false);
+
   EXPECT_EQ(NumBodiesGenerated, 1U);
   EXPECT_EQ(NumPrivatizedVars, 1U);
   EXPECT_EQ(NumFinalizationPoints, 1U);
@@ -372,12 +372,10 @@ TEST_F(OpenMPIRBuilderTest, ParallelSimple) {
   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_FALSE(verifyModule(*M));
   EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoUnwind));
   EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoRecurse));
   EXPECT_TRUE(OutlinedFn->hasParamAttribute(0, Attribute::NoAlias));
@@ -472,7 +470,6 @@ TEST_F(OpenMPIRBuilderTest, ParallelIfCond) {
 
   Builder.restoreIP(AfterIP);
   Builder.CreateRetVoid();
-  OMPBuilder.finalize();
 
   EXPECT_NE(PrivAI, nullptr);
   Function *OutlinedFn = PrivAI->getFunction();
@@ -598,7 +595,6 @@ TEST_F(OpenMPIRBuilderTest, ParallelCancelBarrier) {
 
   Builder.restoreIP(AfterIP);
   Builder.CreateRetVoid();
-  OMPBuilder.finalize();
 
   EXPECT_FALSE(verifyModule(*M, &errs()));