Index: llvm/include/llvm/Transforms/Utils/Cloning.h =================================================================== --- llvm/include/llvm/Transforms/Utils/Cloning.h +++ llvm/include/llvm/Transforms/Utils/Cloning.h @@ -119,24 +119,45 @@ /// values. The final argument captures information about the cloned code if /// non-null. /// -/// VMap contains no non-identity GlobalValue mappings and debug info metadata -/// will not be cloned. +/// \pre VMap contains no non-identity GlobalValue mappings. /// Function *CloneFunction(Function *F, ValueToValueMapTy &VMap, ClonedCodeInfo *CodeInfo = nullptr); +enum class CloneFunctionChangeType { + LocalChangesOnly, + GlobalChanges, + DifferentModule, + ClonedModule, +}; + /// Clone OldFunc into NewFunc, transforming the old arguments into references /// to VMap values. Note that if NewFunc already has basic blocks, the ones /// cloned into it will be added to the end of the function. This function /// fills in a list of return instructions, and can optionally remap types /// and/or append the specified suffix to all values cloned. /// -/// If ModuleLevelChanges is false, VMap contains no non-identity GlobalValue -/// mappings. +/// If \p Changes is \a CloneFunctionChangeType::LocalChangesOnly, VMap is +/// required to contain no non-identity GlobalValue mappings. Otherwise, +/// referenced metadata will be cloned. +/// +/// If \p Changes is less than \a CloneFunctionChangeType::DifferentModule +/// indicating cloning into the same module (even if it's LocalChangesOnly), if +/// debug info metadata transitively references a \a DISubprogram, it will be +/// cloned, effectively upgrading \p Changes to GlobalChanges while suppressing +/// cloning of types and compile units. +/// +/// If \p Changes is \a CloneFunctionChangeType::DifferentModule, the new +/// module's \c !llvm.dbg.cu will get updated with any newly created compile +/// units. (\a CloneFunctionChangeType::ClonedModule leaves that work for the +/// caller.) /// +/// FIXME: Consider simplifying this function by splitting out \a +/// CloneFunctionMetadataInto() and expecting / updating callers to call it +/// first when / how it's needed. void CloneFunctionInto(Function *NewFunc, const Function *OldFunc, - ValueToValueMapTy &VMap, bool ModuleLevelChanges, - SmallVectorImpl &Returns, + ValueToValueMapTy &VMap, CloneFunctionChangeType Changes, + SmallVectorImpl &Returns, const char *NameSuffix = "", ClonedCodeInfo *CodeInfo = nullptr, ValueMapTypeRemapper *TypeMapper = nullptr, Index: llvm/lib/ExecutionEngine/Orc/IndirectionUtils.cpp =================================================================== --- llvm/lib/ExecutionEngine/Orc/IndirectionUtils.cpp +++ llvm/lib/ExecutionEngine/Orc/IndirectionUtils.cpp @@ -316,8 +316,9 @@ "modules."); SmallVector Returns; // Ignore returns cloned. - CloneFunctionInto(NewF, &OrigF, VMap, /*ModuleLevelChanges=*/true, Returns, - "", nullptr, nullptr, Materializer); + CloneFunctionInto(NewF, &OrigF, VMap, + CloneFunctionChangeType::DifferentModule, Returns, "", + nullptr, nullptr, Materializer); OrigF.deleteBody(); } Index: llvm/lib/Target/AMDGPU/R600OpenCLImageTypeLoweringPass.cpp =================================================================== --- llvm/lib/Target/AMDGPU/R600OpenCLImageTypeLoweringPass.cpp +++ llvm/lib/Target/AMDGPU/R600OpenCLImageTypeLoweringPass.cpp @@ -301,7 +301,8 @@ } } SmallVector Returns; - CloneFunctionInto(NewF, F, VMap, /*ModuleLevelChanges=*/false, Returns); + CloneFunctionInto(NewF, F, VMap, CloneFunctionChangeType::LocalChangesOnly, + Returns); // Build new MDNode. SmallVector KernelMDArgs; Index: llvm/lib/Transforms/Coroutines/CoroSplit.cpp =================================================================== --- llvm/lib/Transforms/Coroutines/CoroSplit.cpp +++ llvm/lib/Transforms/Coroutines/CoroSplit.cpp @@ -828,7 +828,8 @@ auto savedLinkage = NewF->getLinkage(); NewF->setLinkage(llvm::GlobalValue::ExternalLinkage); - CloneFunctionInto(NewF, &OrigF, VMap, /*ModuleLevelChanges=*/true, Returns); + CloneFunctionInto(NewF, &OrigF, VMap, + CloneFunctionChangeType::LocalChangesOnly, Returns); NewF->setLinkage(savedLinkage); NewF->setVisibility(savedVisibility); Index: llvm/lib/Transforms/IPO/Attributor.cpp =================================================================== --- llvm/lib/Transforms/IPO/Attributor.cpp +++ llvm/lib/Transforms/IPO/Attributor.cpp @@ -1527,7 +1527,8 @@ SmallVector Returns; // Copy the body of the original function to the new one - CloneFunctionInto(Copied, &F, VMap, /* ModuleLevelChanges */ false, Returns); + CloneFunctionInto(Copied, &F, VMap, CloneFunctionChangeType::LocalChangesOnly, + Returns); // Set the linakage and visibility late as CloneFunctionInto has some implicit // requirements. Index: llvm/lib/Transforms/Utils/CloneFunction.cpp =================================================================== --- llvm/lib/Transforms/Utils/CloneFunction.cpp +++ llvm/lib/Transforms/Utils/CloneFunction.cpp @@ -83,8 +83,8 @@ // void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, ValueToValueMapTy &VMap, - bool ModuleLevelChanges, - SmallVectorImpl &Returns, + CloneFunctionChangeType Changes, + SmallVectorImpl &Returns, const char *NameSuffix, ClonedCodeInfo *CodeInfo, ValueMapTypeRemapper *TypeMapper, ValueMaterializer *Materializer) { @@ -95,6 +95,8 @@ assert(VMap.count(&I) && "No mapping from source argument specified!"); #endif + bool ModuleLevelChanges = Changes > CloneFunctionChangeType::LocalChangesOnly; + // Copy all attributes other than those stored in the AttributeList. We need // to remap the parameter indices of the AttributeList. AttributeList NewAttrs = NewFunc->getAttributes(); @@ -123,21 +125,37 @@ AttributeList::get(NewFunc->getContext(), OldAttrs.getFnAttributes(), OldAttrs.getRetAttributes(), NewArgAttrs)); - bool MustCloneSP = - OldFunc->getParent() && OldFunc->getParent() == NewFunc->getParent(); - DISubprogram *SP = OldFunc->getSubprogram(); - if (SP) { - assert(!MustCloneSP || ModuleLevelChanges); - // Add mappings for some DebugInfo nodes that we don't want duplicated - // even if they're distinct. - auto &MD = VMap.MD(); - MD[SP->getUnit()].reset(SP->getUnit()); - MD[SP->getType()].reset(SP->getType()); - MD[SP->getFile()].reset(SP->getFile()); - // If we're not cloning into the same module, no need to clone the - // subprogram - if (!MustCloneSP) - MD[SP].reset(SP); + // When we remap instructions within the same module, we want to avoid + // duplicating inlined DISubprograms, so record all subprograms we find as we + // duplicate instructions and then freeze them in the MD map. We also record + // information about dbg.value and dbg.declare to avoid duplicating the + // types. + Optional DIFinder; + + // Track the subprogram attachment that needs to be cloned to fine-tune the + // mapping within the same module. + DISubprogram *SPClonedWithinModule = nullptr; + if (Changes < CloneFunctionChangeType::DifferentModule) { + assert((NewFunc->getParent() == nullptr || + NewFunc->getParent() == OldFunc->getParent()) && + "Expected NewFunc to have the same parent, or no parent"); + + // Need to find subprograms, types, and compile units. + DIFinder.emplace(); + + SPClonedWithinModule = OldFunc->getSubprogram(); + } else { + assert((NewFunc->getParent() == nullptr || + NewFunc->getParent() != OldFunc->getParent()) && + "Set SameModule to true if the new function is in the same module"); + + if (Changes == CloneFunctionChangeType::DifferentModule) { + assert(NewFunc->getParent() && + "Need parent of new function to maintain debug info invariants"); + + // Need to find all the compile units. + DIFinder.emplace(); + } } // Everything else beyond this point deals with function instructions, @@ -145,13 +163,6 @@ if (OldFunc->isDeclaration()) return; - // When we remap instructions, we want to avoid duplicating inlined - // DISubprograms, so record all subprograms we find as we duplicate - // instructions and then freeze them in the MD map. - // We also record information about dbg.value and dbg.declare to avoid - // duplicating the types. - DebugInfoFinder DIFinder; - // Loop over all of the basic blocks in the function, cloning them as // appropriate. Note that we save BE this way in order to handle cloning of // recursive functions into themselves. @@ -161,7 +172,7 @@ // Create a new basic block and copy instructions into it! BasicBlock *CBB = CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo, - ModuleLevelChanges ? &DIFinder : nullptr); + DIFinder ? &*DIFinder : nullptr); // Add basic block mapping. VMap[&BB] = CBB; @@ -183,15 +194,38 @@ Returns.push_back(RI); } - for (DISubprogram *ISP : DIFinder.subprograms()) - if (ISP != SP) - VMap.MD()[ISP].reset(ISP); + if (Changes < CloneFunctionChangeType::DifferentModule && + (SPClonedWithinModule || DIFinder->subprogram_count() > 0)) { + // Turn on module-level changes, since we need to clone (some of) the + // debug info metadata. + // + // FIXME: Metadata effectively owned by a function should be made + // local, and only that local metadata should be cloned. + ModuleLevelChanges = true; + + auto mapToSelfIfNew = [&VMap](MDNode *N) { + // Avoid clobbering an existing mapping. + (void)VMap.MD().try_emplace(N, N); + }; + + // Avoid cloning what the subprogram references. + if (SPClonedWithinModule) { + mapToSelfIfNew(SPClonedWithinModule->getUnit()); + mapToSelfIfNew(SPClonedWithinModule->getType()); + mapToSelfIfNew(SPClonedWithinModule->getFile()); + } + + // Avoid cloning other subprograms, compile units, and types. + for (DISubprogram *ISP : DIFinder->subprograms()) + if (ISP != SPClonedWithinModule) + mapToSelfIfNew(ISP); - for (DICompileUnit *CU : DIFinder.compile_units()) - VMap.MD()[CU].reset(CU); + for (DICompileUnit *CU : DIFinder->compile_units()) + mapToSelfIfNew(CU); - for (DIType *Type : DIFinder.types()) - VMap.MD()[Type].reset(Type); + for (DIType *Type : DIFinder->types()) + mapToSelfIfNew(Type); + } // Duplicate the metadata that is attached to the cloned function. // Subprograms/CUs/types that were already mapped to themselves won't be @@ -218,19 +252,30 @@ ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges, TypeMapper, Materializer); - // Register all DICompileUnits of the old parent module in the new parent module - auto* OldModule = OldFunc->getParent(); + if (Changes != CloneFunctionChangeType::DifferentModule) + return; + + // Update !llvm.dbg.cu with compile units added to the new module if this + // function is being cloned in isolation. + // + // FIXME: This is making global / module-level changes, which doesn't seem + // like the right encapsulation Consider dropping the requirement to update + // !llvm.dbg.cu (either obsoleting the node, or restricting it to + // non-discardable compile units) instead of discovering compile units by + // visiting the metadata attached to global values, which would allow this + // code to be deleted. Alternatively, perhaps give responsibility for this + // update to CloneFunctionInto's callers. auto* NewModule = NewFunc->getParent(); - if (OldModule && NewModule && OldModule != NewModule && DIFinder.compile_unit_count()) { - auto* NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu"); - // Avoid multiple insertions of the same DICompileUnit to NMD. - SmallPtrSet Visited; - for (auto* Operand : NMD->operands()) - Visited.insert(Operand); - for (auto* Unit : DIFinder.compile_units()) - // VMap.MD()[Unit] == Unit - if (Visited.insert(Unit).second) - NMD->addOperand(Unit); + auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu"); + // Avoid multiple insertions of the same DICompileUnit to NMD. + SmallPtrSet Visited; + for (auto *Operand : NMD->operands()) + Visited.insert(Operand); + for (auto *Unit : DIFinder->compile_units()) { + MDNode *MappedUnit = + MapMetadata(Unit, VMap, RF_None, TypeMapper, Materializer); + if (Visited.insert(MappedUnit).second) + NMD->addOperand(MappedUnit); } } @@ -269,8 +314,8 @@ } SmallVector Returns; // Ignore returns cloned. - CloneFunctionInto(NewF, F, VMap, F->getSubprogram() != nullptr, Returns, "", - CodeInfo); + CloneFunctionInto(NewF, F, VMap, CloneFunctionChangeType::LocalChangesOnly, + Returns, "", CodeInfo); return NewF; } Index: llvm/lib/Transforms/Utils/CloneModule.cpp =================================================================== --- llvm/lib/Transforms/Utils/CloneModule.cpp +++ llvm/lib/Transforms/Utils/CloneModule.cpp @@ -120,13 +120,8 @@ SmallVector, 1> MDs; G.getAllMetadata(MDs); - - // FIXME: Stop using RF_ReuseAndMutateDistinctMDs here, since it's unsound - // to mutate metadata that is still referenced by the source module unless - // the source is about to be discarded (see IRMover for a valid use). for (auto MD : MDs) - GV->addMetadata(MD.first, *MapMetadata(MD.second, VMap, - RF_ReuseAndMutateDistinctMDs)); + GV->addMetadata(MD.first, *MapMetadata(MD.second, VMap)); if (G.isDeclaration()) continue; @@ -165,7 +160,8 @@ } SmallVector Returns; // Ignore returns cloned. - CloneFunctionInto(F, &I, VMap, /*ModuleLevelChanges=*/true, Returns); + CloneFunctionInto(F, &I, VMap, CloneFunctionChangeType::ClonedModule, + Returns); if (I.hasPersonalityFn()) F->setPersonalityFn(MapValue(I.getPersonalityFn(), VMap)); @@ -185,25 +181,13 @@ } // And named metadata.... - const auto* LLVM_DBG_CU = M.getNamedMetadata("llvm.dbg.cu"); for (Module::const_named_metadata_iterator I = M.named_metadata_begin(), E = M.named_metadata_end(); I != E; ++I) { const NamedMDNode &NMD = *I; NamedMDNode *NewNMD = New->getOrInsertNamedMetadata(NMD.getName()); - if (&NMD == LLVM_DBG_CU) { - // Do not insert duplicate operands. - SmallPtrSet Visited; - for (const auto* Operand : NewNMD->operands()) - Visited.insert(Operand); - for (const auto* Operand : NMD.operands()) { - auto* MappedOperand = MapMetadata(Operand, VMap); - if (Visited.insert(MappedOperand).second) - NewNMD->addOperand(MappedOperand); - } - } else - for (unsigned i = 0, e = NMD.getNumOperands(); i != e; ++i) - NewNMD->addOperand(MapMetadata(NMD.getOperand(i), VMap)); + for (unsigned i = 0, e = NMD.getNumOperands(); i != e; ++i) + NewNMD->addOperand(MapMetadata(NMD.getOperand(i), VMap)); } return New; Index: llvm/unittests/Transforms/Utils/CloningTest.cpp =================================================================== --- llvm/unittests/Transforms/Utils/CloningTest.cpp +++ llvm/unittests/Transforms/Utils/CloningTest.cpp @@ -177,7 +177,8 @@ ValueToValueMapTy VMap; VMap[A] = UndefValue::get(A->getType()); - CloneFunctionInto(F2, F1, VMap, false, Returns); + CloneFunctionInto(F2, F1, VMap, CloneFunctionChangeType::LocalChangesOnly, + Returns); EXPECT_FALSE(F2->arg_begin()->hasNoCaptureAttr()); delete F1; @@ -200,7 +201,8 @@ ValueToValueMapTy VMap; VMap[&*F1->arg_begin()] = &*F2->arg_begin(); - CloneFunctionInto(F2, F1, VMap, false, Returns); + CloneFunctionInto(F2, F1, VMap, CloneFunctionChangeType::LocalChangesOnly, + Returns); EXPECT_EQ(CallingConv::Cold, F2->getCallingConv()); delete F1; @@ -663,6 +665,28 @@ return 0; } +static bool haveCompileUnitsInCommon(const Module &LHS, const Module &RHS) { + const NamedMDNode *LHSCUs = LHS.getNamedMetadata("llvm.dbg.cu"); + if (!LHSCUs) + return false; + + const NamedMDNode *RHSCUs = RHS.getNamedMetadata("llvm.dbg.cu"); + if (!RHSCUs) + return false; + + SmallPtrSet Found; + for (int I = 0, E = LHSCUs->getNumOperands(); I != E; ++I) + if (const MDNode *N = LHSCUs->getOperand(I)) + Found.insert(N); + + for (int I = 0, E = RHSCUs->getNumOperands(); I != E; ++I) + if (const MDNode *N = RHSCUs->getOperand(I)) + if (Found.count(N)) + return true; + + return false; +} + TEST(CloneFunction, CloneEmptyFunction) { StringRef ImplAssembly = R"( define void @foo() { @@ -684,7 +708,8 @@ ValueToValueMapTy VMap; SmallVector Returns; ClonedCodeInfo CCI; - CloneFunctionInto(ImplFunction, DeclFunction, VMap, true, Returns, "", &CCI); + CloneFunctionInto(ImplFunction, DeclFunction, VMap, + CloneFunctionChangeType::GlobalChanges, Returns, "", &CCI); EXPECT_FALSE(verifyModule(*ImplModule, &errs())); EXPECT_FALSE(CCI.ContainsCalls); @@ -715,7 +740,8 @@ ValueToValueMapTy VMap; SmallVector Returns; ClonedCodeInfo CCI; - CloneFunctionInto(DeclFunction, ImplFunction, VMap, true, Returns, "", &CCI); + CloneFunctionInto(DeclFunction, ImplFunction, VMap, + CloneFunctionChangeType::GlobalChanges, Returns, "", &CCI); EXPECT_FALSE(verifyModule(*ImplModule, &errs())); EXPECT_TRUE(CCI.ContainsCalls); @@ -764,7 +790,8 @@ ValueToValueMapTy VMap; SmallVector Returns; ClonedCodeInfo CCI; - CloneFunctionInto(NewFunc, OldFunc, VMap, true, Returns, "", &CCI); + CloneFunctionInto(NewFunc, OldFunc, VMap, + CloneFunctionChangeType::GlobalChanges, Returns, "", &CCI); // This fails if the scopes in the llvm.dbg.declare variable and location // aren't the same. @@ -812,12 +839,14 @@ VMap[ImplFunction] = DeclFunction; // No args to map SmallVector Returns; - CloneFunctionInto(DeclFunction, ImplFunction, VMap, true, Returns); + CloneFunctionInto(DeclFunction, ImplFunction, VMap, + CloneFunctionChangeType::DifferentModule, Returns); EXPECT_FALSE(verifyModule(*ImplModule, &errs())); EXPECT_FALSE(verifyModule(*DeclModule, &errs())); - // DICompileUnit !2 shall be inserted into DeclModule. + // DICompileUnit !2 shall be cloned into DeclModule. EXPECT_TRUE(GetDICompileUnitCount(*DeclModule) == 1); + EXPECT_FALSE(haveCompileUnitsInCommon(*ImplModule, *DeclModule)); } class CloneModule : public ::testing::Test { @@ -945,6 +974,7 @@ auto *NMD = NewM->getNamedMetadata("llvm.dbg.cu"); EXPECT_TRUE(NMD != nullptr); EXPECT_EQ(NMD->getNumOperands(), 1U); + EXPECT_FALSE(haveCompileUnitsInCommon(*OldM, *NewM)); DICompileUnit *CU = dyn_cast(NMD->getOperand(0)); EXPECT_TRUE(CU != nullptr);