From: Johannes Doerfert Date: Thu, 13 Feb 2020 00:50:22 +0000 (-0600) Subject: Revert "[OpenMP][IRBuilder] Perform finalization (incl. outlining) late" X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=3aac953afa34885a72df96f2b703b65f85cbb149;p=platform%2Fupstream%2Fllvm.git Revert "[OpenMP][IRBuilder] Perform finalization (incl. outlining) late" This reverts commit 8a56d64d7620b3764f10f03f3a1e307fcdd72c2f. Will be recommitted once the clang test problem is addressed. --- diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp index 4798de0..b7506b5 100644 --- 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 index a1470bc..07ec82d 100644 --- a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h +++ b/llvm/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, GlobalVariable *> IdentMap; - - /// Helper that contains information about regions we need to outline - /// during finalization. - struct OutlineInfo { - SmallVector Blocks; - using PostOutlineCBTy = std::function; - PostOutlineCBTy PostOutlineCB; - }; - - /// Collection of regions that need to be outlined during finalization. - SmallVector 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 index 2418459..d45d1d0 100644 --- 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,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 ParallelRegionBlockSet; - SmallVector Worklist; + SmallVector 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(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 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(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( - 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(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 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(>id, &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(>id, &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 && @@ -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(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 index e72e17f..210cd7d 100644 --- 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 index 4bbd966..c6a51f6 100644 --- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp +++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp @@ -96,7 +96,7 @@ TEST_F(OpenMPIRBuilderTest, CreateBarrier) { EXPECT_EQ(cast(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 = - 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); @@ -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()));