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

jdoerfert · jdoerfert · commit 3aac953afa34 · 2020-02-12T18:50:43.000-06:00
This reverts commit 8a56d64. Will be recommitted once the clang test problem is addressed.
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -32,7 +32,6 @@
 #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"
@@ -105,14 +104,6 @@ CodeGenFunction::~CodeGenFunction() {
 
   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
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -34,9 +34,6 @@ class OpenMPIRBuilder {
   /// 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 @@ class OpenMPIRBuilder {
 
   /// 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
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/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,33 +415,32 @@ 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);
 
   // Collect all blocks in-between PRegEntryBB and PRegExitBB.
   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,104 +558,75 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
       //    callback callee.
       F->addMetadata(
           llvm::LLVMContext::MD_callback,
-          *llvm::MDNode::get(
-              Ctx, {MDB.createCallbackEncoding(2, {-1, -1},
-                                               /* VarArgsArePassed */ true)}));
+          *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);
 
-    Builder.CreateCall(RTLFn, RealArgs);
+  LLVM_DEBUG(dbgs() << "With fork_call placed: "
+                    << *Builder.GetInsertBlock()->getParent() << "\n");
 
-    LLVM_DEBUG(dbgs() << "With fork_call placed: "
-                      << *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);
+  InsertPointTy AfterIP(UI->getParent(), UI->getParent()->end());
+  InsertPointTy ExitIP(PRegExitBB, PRegExitBB->end());
+  UI->eraseFromParent();
 
-    // 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 {
+  // 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 an "if" clause was present we are now generating the serialized
-      // version into the "else" branch.
-      Builder.SetInsertPoint(ElseTI);
+  // 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 {
 
-      // Build calls __kmpc_serialized_parallel(&Ident, GTid);
-      Value *SerializedParallelCallArgs[] = {Ident, ThreadID};
-      Builder.CreateCall(
-          getOrCreateRuntimeFunction(OMPRTL___kmpc_serialized_parallel),
-          SerializedParallelCallArgs);
+    // If an "if" clause was present we are now generating the serialized
+    // version into the "else" branch.
+    Builder.SetInsertPoint(ElseTI);
 
-      // OutlinedFn(&GTid, &zero, CapturedStruct);
-      CI->removeFromParent();
-      Builder.Insert(CI);
+    // Build calls __kmpc_serialized_parallel(&Ident, GTid);
+    Value *SerializedParallelCallArgs[] = {Ident, ThreadID};
+    Builder.CreateCall(
+        getOrCreateRuntimeFunction(OMPRTL___kmpc_serialized_parallel),
+        SerializedParallelCallArgs);
 
-      // __kmpc_end_serialized_parallel(&Ident, GTid);
-      Value *EndArgs[] = {Ident, ThreadID};
-      Builder.CreateCall(
-          getOrCreateRuntimeFunction(OMPRTL___kmpc_end_serialized_parallel),
-          EndArgs);
+    // OutlinedFn(&GTid, &zero, CapturedStruct);
+    CI->removeFromParent();
+    Builder.Insert(CI);
 
-      LLVM_DEBUG(dbgs() << "With serialized parallel region: "
-                        << *Builder.GetInsertBlock()->getParent() << "\n");
-    }
+    // __kmpc_end_serialized_parallel(&Ident, GTid);
+    Value *EndArgs[] = {Ident, ThreadID};
+    Builder.CreateCall(
+        getOrCreateRuntimeFunction(OMPRTL___kmpc_end_serialized_parallel),
+        EndArgs);
 
-    for (Instruction *I : ToBeDeleted)
-      I->eraseFromParent();
-  };
+    LLVM_DEBUG(dbgs() << "With serialized parallel region: "
+                      << *Builder.GetInsertBlock()->getParent() << "\n");
+  }
 
   // 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) == 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());
-  UI->eraseFromParent();
-
-  // Register the outlined info.
-  addOutlineInfo(std::move(OI));
+  for (Instruction *I : ToBeDeleted)
+    I->eraseFromParent();
 
   return AfterIP;
 }
diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/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.
diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
