Cloning: Clean up the interface to the CloneFunction function.

[pcc]

Summary

Remove the ModuleLevelChanges argument, and the ability to create new
subprograms for cloned functions. The latter was added without review in
r203662, but it has no in-tree clients (all non-test callers pass false
for ModuleLevelChanges [1], so it isn't reachable outside of tests). It
also isn't clear that adding a duplicate subprogram to the compile unit is
always the right thing to do when cloning a function within a module. If
this functionality comes back it should be accompanied with a more concrete
use case.

Furthermore, all in-tree clients add the returned function to the module.
Since that's pretty much the only sensible thing you can do with the function,
just do that in CloneFunction.

[1] http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/CloneFunction.cpp/rCloneFunction

[dexonsmith]

[e-mail], who committed r203662.

llvm/trunk/include/llvm/Transforms/Utils/Cloning.h

 /// If you would like to collect additional information about the cloned
 /// function, you can specify a ClonedCodeInfo object with the optional fifth
 /// parameter.
 ///
 BasicBlock *CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
                             const Twine &NameSuffix = "", Function *F = nullptr,
                             ClonedCodeInfo *CodeInfo = nullptr);
 
-/// CloneFunction - Return a copy of the specified function, but without
-/// embedding the function into another module.  Also, any references specified
-/// in the VMap are changed to refer to their mapped value instead of the
-/// original one.  If any of the arguments to the function are in the VMap,
-/// the arguments are deleted from the resultant function.  The VMap is
-/// updated to include mappings from all of the instructions and basicblocks in
-/// the function from their old to new values.  The final argument captures
-/// information about the cloned code if non-null.
+/// CloneFunction - Return a copy of the specified function and add it to that
+/// function's module.  Also, any references specified in the VMap are changed
+/// to refer to their mapped value instead of the original one.  If any of the
+/// arguments to the function are in the VMap, the arguments are deleted from
+/// the resultant function.  The VMap is updated to include mappings from all of
+/// the instructions and basicblocks in the function from their old to new
+/// values.  The final argument captures information about the cloned code if
+/// non-null.
 ///
-/// If ModuleLevelChanges is false, VMap contains no non-identity GlobalValue
-/// mappings, and debug info metadata will not be cloned.
+/// VMap contains no non-identity GlobalValue mappings and debug info metadata
+/// will not be cloned.
 ///
-Function *CloneFunction(const Function *F, ValueToValueMapTy &VMap,
-                        bool ModuleLevelChanges,
+Function *CloneFunction(Function *F, ValueToValueMapTy &VMap,
                         ClonedCodeInfo *CodeInfo = nullptr);
 
 /// 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.
 ///

llvm/trunk/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp

bool AMDGPUAlwaysInline::runOnModule(Module &M) {
   for (Function &F : M) {
     if (!F.hasLocalLinkage() && !F.isDeclaration() && !F.use_empty() &&
         !F.hasFnAttribute(Attribute::NoInline))
       FuncsToClone.push_back(&F);
   }
 
   for (Function *F : FuncsToClone) {
     ValueToValueMapTy VMap;
-    Function *NewFunc = CloneFunction(F, VMap, false);
+    Function *NewFunc = CloneFunction(F, VMap);
     NewFunc->setLinkage(GlobalValue::InternalLinkage);
-    M.getFunctionList().push_back(NewFunc);
     F->replaceAllUsesWith(NewFunc);
   }
 
   for (Function &F : M) {
     if (F.hasLocalLinkage() && !F.hasFnAttribute(Attribute::NoInline)) {
       F.addFnAttr(Attribute::AlwaysInline);
     }
   }
   return false;
 }
 
 ModulePass *llvm::createAMDGPUAlwaysInlinePass() {
   return new AMDGPUAlwaysInline();
 }

llvm/trunk/lib/Transforms/IPO/PartialInlining.cpp

if (isa<ReturnInst>(BB->getTerminator())) {
       nonReturnBlock = BB;
   }
   
   if (returnCount != 1)
     return nullptr;
   
   // Clone the function, so that we can hack away on it.
   ValueToValueMapTy VMap;
-  Function* duplicateFunction = CloneFunction(F, VMap,
-                                              /*ModuleLevelChanges=*/false);
+  Function* duplicateFunction = CloneFunction(F, VMap);
   duplicateFunction->setLinkage(GlobalValue::InternalLinkage);
-  F->getParent()->getFunctionList().push_back(duplicateFunction);
   BasicBlock* newEntryBlock = cast<BasicBlock>(VMap[entryBlock]);
   BasicBlock* newReturnBlock = cast<BasicBlock>(VMap[returnBlock]);
   BasicBlock* newNonReturnBlock = cast<BasicBlock>(VMap[nonReturnBlock]);
   
   // Go ahead and update all uses to the duplicate, so that we can just
   // use the inliner functionality when we're done hacking.
   F->replaceAllUsesWith(duplicateFunction);
   

llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp

for (Function::iterator BB =
        BB != BE; ++BB)
     // Loop over all instructions, fixing each one as we find it...
     for (Instruction &II : *BB)
       RemapInstruction(&II, VMap,
                        ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
                        TypeMapper, Materializer);
 }
 
-// Clone the module-level debug info associated with OldFunc. The cloned data
-// will point to NewFunc instead.
-static void CloneDebugInfoMetadata(Function *NewFunc, const Function *OldFunc,
-                                   ValueToValueMapTy &VMap) {
-  if (const DISubprogram *OldSP = OldFunc->getSubprogram()) {
-    auto *NewSP = cast<DISubprogram>(MapMetadata(OldSP, VMap));
-    // FIXME: There ought to be a better way to do this: ValueMapper
-    // will clone the distinct DICompileUnit. Use the original one
-    // instead.
-    NewSP->replaceUnit(OldSP->getUnit());
-    NewFunc->setSubprogram(NewSP);
-  }
-}
-
-/// Return a copy of the specified function, but without
-/// embedding the function into another module.  Also, any references specified
-/// in the VMap are changed to refer to their mapped value instead of the
-/// original one.  If any of the arguments to the function are in the VMap,
-/// the arguments are deleted from the resultant function.  The VMap is
-/// updated to include mappings from all of the instructions and basicblocks in
-/// the function from their old to new values.
+/// Return a copy of the specified function and add it to that function's
+/// module.  Also, any references specified in the VMap are changed to refer to
+/// their mapped value instead of the original one.  If any of the arguments to
+/// the function are in the VMap, the arguments are deleted from the resultant
+/// function.  The VMap is updated to include mappings from all of the
+/// instructions and basicblocks in the function from their old to new values.
 ///
-Function *llvm::CloneFunction(const Function *F, ValueToValueMapTy &VMap,
-                              bool ModuleLevelChanges,
+Function *llvm::CloneFunction(Function *F, ValueToValueMapTy &VMap,
                               ClonedCodeInfo *CodeInfo) {
   std::vector<Type*> ArgTypes;
 
   // The user might be deleting arguments to the function by specifying them in
   // the VMap.  If so, we need to not add the arguments to the arg ty vector
   //
   for (const Argument &I : F->args())
     if (VMap.count(&I) == 0) // Haven't mapped the argument to anything yet?
       ArgTypes.push_back(I.getType());
 
   // Create a new function type...
   FunctionType *FTy = FunctionType::get(F->getFunctionType()->getReturnType(),
                                     ArgTypes, F->getFunctionType()->isVarArg());
 
   // Create the new function...
-  Function *NewF = Function::Create(FTy, F->getLinkage(), F->getName());
+  Function *NewF =
+      Function::Create(FTy, F->getLinkage(), F->getName(), F->getParent());
 
   // Loop over the arguments, copying the names of the mapped arguments over...
   Function::arg_iterator DestI = NewF->arg_begin();
   for (const Argument & I : F->args())
     if (VMap.count(&I) == 0) {     // Is this argument preserved?
       DestI->setName(I.getName()); // Copy the name over...
       VMap[&I] = &*DestI++;        // Add mapping to VMap
     }
 
-  if (ModuleLevelChanges)
-    CloneDebugInfoMetadata(NewF, F, VMap);
-
   SmallVector<ReturnInst*, 8> Returns;  // Ignore returns cloned.
-  CloneFunctionInto(NewF, F, VMap, ModuleLevelChanges, Returns, "", CodeInfo);
+  CloneFunctionInto(NewF, F, VMap, /*ModuleLevelChanges=*/false, Returns, "",
+                    CodeInfo);
+
   return NewF;
 }
 
 
 
 namespace {
   /// This is a private class used to implement CloneAndPruneFunctionInto.
   struct PruningFunctionCloner {

llvm/trunk/unittests/Transforms/Utils/Cloning.cpp

void CreateOldFunctionBodyAndDI() {
     DIBuilder DBuilder2(*M);
     DBuilder2.createCompileUnit(dwarf::DW_LANG_C99,
         "extra.c", "/file/dir", "CloneFunc", false, "", 0);
     DBuilder2.finalize();
   }
 
   void CreateNewFunc() {
     ValueToValueMapTy VMap;
-    NewFunc = CloneFunction(OldFunc, VMap, true, nullptr);
-    M->getFunctionList().push_back(NewFunc);
+    NewFunc = CloneFunction(OldFunc, VMap, nullptr);
   }
 
   void SetupFinder() {
     Finder = new DebugInfoFinder();
     Finder->processModule(*M);
   }
 
   LLVMContext C;

 }
 
 // Test that a new subprogram entry was added and is pointing to the new
 // function, while the original subprogram still points to the old one.
 TEST_F(CloneFunc, Subprogram) {
   EXPECT_FALSE(verifyModule(*M));
 
   unsigned SubprogramCount = Finder->subprogram_count();
-  EXPECT_EQ(2U, SubprogramCount);
+  EXPECT_EQ(1U, SubprogramCount);
 
   auto Iter = Finder->subprograms().begin();
-  auto *Sub1 = cast<DISubprogram>(*Iter);
-  Iter++;
-  auto *Sub2 = cast<DISubprogram>(*Iter);
-
-  EXPECT_TRUE(
-      (Sub1 == OldFunc->getSubprogram() && Sub2 == NewFunc->getSubprogram()) ||
-      (Sub1 == NewFunc->getSubprogram() && Sub2 == OldFunc->getSubprogram()));
+  auto *Sub = cast<DISubprogram>(*Iter);
+
+  EXPECT_TRUE(Sub == OldFunc->getSubprogram());
+  EXPECT_TRUE(Sub == NewFunc->getSubprogram());
 }
 
 // Test that instructions in the old function still belong to it in the
 // metadata, while instruction in the new function belong to the new one.
 TEST_F(CloneFunc, InstructionOwnership) {
   EXPECT_FALSE(verifyModule(*M));
 
   inst_iterator OldIter = inst_begin(OldFunc);