Fix DISubprogram while extracting instructions out of function

[hiraditya]

Summary

This case triggers when merge-similar-functions tries to extract instructions out of instantiations of a template function.

[aprantl]

Looks generally good, some nitpicks inside.

include/llvm/Transforms/Utils/Cloning.h

161

Can you add an

enum {
  ExtractingFunctions = true;
  CloningIntoSameModule = false;
};

or something along those lines for better readability?

test/Transforms/MergeSimilarFunc/merge-debug-info-2.ll

59

Is there a reason why this function needs to have more than just a single instruction?

69

please delete all the !tbaa metadata it's just distracting and will make the test harder to update in the future.

86

please delete all non-essential attributes (everything in quotes).

[tobiasvk]

lib/Transforms/Utils/CloneFunction.cpp

136

How about changing this to MustCloneSP = !ExtractingFunction && OldFunc->getParent() && OldFunc->getParent() == NewFunc->getParent(). Then you can leave the if block right after unchanged.

[hiraditya]

test/Transforms/MergeSimilarFunc/merge-debug-info-2.ll

59

Merge function seem to have some size metric to merge instructions. I think it may be possible to reduce a little bit but how much I'm not sure.

[hiraditya]

Addressed comments

[dblaikie]

You can use attributes to force inlining or non-inlining to create a
particular scenario.

[aprantl]

include/llvm/Transforms/Utils/Cloning.h

132

Can you please also add documentation to CloneFunctionInto that explains what the difference between those modes is?

132

/// Used to control \p CloneFunctionInto.

[hiraditya]

Comments for enum

[aprantl]

include/llvm/Transforms/Utils/Cloning.h

132

/// Used to control \p CloneFunctionInto.
enum class CloneType {
    InvalidCloneType,
    /// Cloning will result in module level changes.
    ModuleLevelChanges,
    /// What does this mean exactly? Do we even need it?
    NoModuleLevelChanges,
    /// Cloning will be used for extracting functions
    /// by passes like function merging, it does not require module level changes
    /// but debug info needs special treatment like: DISubprogram is not cloned.
    ExtractingFunctions,
};

[hiraditya]

include/llvm/Transforms/Utils/Cloning.h

132

What does this mean exactly? Do we even need it?

This is used in cases when we dont want module level changes but it is not used for extracting functions. I did not want to modify the behavior at other places so introduced this for example: calls to CloneFunctionInto in Cloning.cpp. Perhaps we can merge ExtractingFunctions with NoModuleLevelChanges but I'm not very sure.

[hiraditya]

Comments

[aprantl]

include/llvm/Transforms/Utils/Cloning.h

137

Could you please comment that explains exactly what scenario this is supposed to be used for? The idea being that If we can't document the use-case precisely, we probably should not have this mode.

include/llvm/Transforms/Utils/Cloning.h

 /// non-null.
 ///
 /// VMap contains no non-identity GlobalValue mappings and debug info metadata
 /// will not be cloned.
 ///
 Function *CloneFunction(Function *F, ValueToValueMapTy &VMap,
                         ClonedCodeInfo *CodeInfo = nullptr);
 
+/// Used to control @fn CloneFunctionInto.
+enum class CloneType {
+    InvalidCloneType,
+    /// Cloning will result in module level changes.
+    ModuleLevelChanges,
+    /// !ModuleLevelChanges
+    NoModuleLevelChanges,
+    /// Cloning will be used for extracting functions
+    /// by passes like function merging, it does not require module level changes
+    /// but debug info needs special treatment like: DISubprogram is not cloned.
+    ExtractingFunctions,
+};
+
 /// 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.
 ///
 void CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
-                       ValueToValueMapTy &VMap, bool ModuleLevelChanges,
+                       ValueToValueMapTy &VMap, CloneType CT,
                        SmallVectorImpl<ReturnInst*> &Returns,
                        const char *NameSuffix = "",
                        ClonedCodeInfo *CodeInfo = nullptr,
                        ValueMapTypeRemapper *TypeMapper = nullptr,
                        ValueMaterializer *Materializer = nullptr);
 
 void CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
                                const Instruction *StartingInst,
                                ValueToValueMapTy &VMap, bool ModuleLevelChanges,
                                SmallVectorImpl<ReturnInst *> &Returns,
                                const char *NameSuffix = "",
                                ClonedCodeInfo *CodeInfo = nullptr);
 
 /// CloneAndPruneFunctionInto - This works exactly like CloneFunctionInto,

lib/ExecutionEngine/Orc/IndirectionUtils.cpp

void moveFunctionBody(Function &OrigF, ValueToValueMapTy &VMap,
   else
     assert(VMap[&OrigF] == NewF && "Incorrect function mapping in VMap.");
   assert(NewF && "Function mapping missing from VMap.");
   assert(NewF->getParent() != OrigF.getParent() &&
          "moveFunctionBody should only be used to move bodies between "
          "modules.");
 
   SmallVector<ReturnInst *, 8> Returns; // Ignore returns cloned.
-  CloneFunctionInto(NewF, &OrigF, VMap, /*ModuleLevelChanges=*/true, Returns,
+  CloneFunctionInto(NewF, &OrigF, VMap, CloneType::ModuleLevelChanges, Returns,
                     "", nullptr, nullptr, Materializer);
   OrigF.deleteBody();
 }
 
 GlobalVariable* cloneGlobalVariableDecl(Module &Dst, const GlobalVariable &GV,
                                         ValueToValueMapTy *VMap) {
   assert(GV.getParent() != &Dst && "Can't copy decl over existing global var.");
   GlobalVariable *NewGV = new GlobalVariable(

lib/Transforms/Coroutines/CoroSplit.cpp

static Function *createClone(Function &F, Twine Suffix, coro::Shape &Shape,
   // Replace all args with undefs. The buildCoroutineFrame algorithm already
   // rewritten access to the args that occurs after suspend points with loads
   // and stores to/from the coroutine frame.
   for (Argument &A : F.args())
     VMap[&A] = UndefValue::get(A.getType());
 
   SmallVector<ReturnInst *, 4> Returns;
 
-  CloneFunctionInto(NewF, &F, VMap, /*ModuleLevelChanges=*/true, Returns);
+  CloneFunctionInto(NewF, &F, VMap, CloneType::ModuleLevelChanges, Returns);
 
   // Remove old returns.
   for (ReturnInst *Return : Returns)
     changeToUnreachable(Return, /*UseLLVMTrap=*/false);
 
   // Remove old return attributes.
   NewF->removeAttributes(
       AttributeList::ReturnIndex,

lib/Transforms/IPO/MergeSimilarFunctions.cpp

     VMap[&Arg] = DestIn;            // Add mapping to VMap
     ++DestI;
   }
 
   // Name the selector argument
   (*DestI).setName("__merge_arg");
 
   SmallVector<ReturnInst*, 8> Returns;
-  CloneFunctionInto(NewF, F1, VMap, false, Returns);
+  CloneFunctionInto(NewF, F1, VMap, CloneType::ExtractingFunctions, Returns);
   // Set linkage to set visibility to default.
   NewF->setLinkage(GlobalValue::InternalLinkage);
 
   return NewF;
 }
 
 typedef MapVector<const Instruction *, std::vector<const Instruction *> >
   CombinedDiffMap;

lib/Transforms/Utils/CloneFunction.cpp

if (CodeInfo) {
     CodeInfo->ContainsDynamicAllocas |= hasStaticAllocas && 
                                         BB != &BB->getParent()->getEntryBlock();
   }
   return NewBB;
 }
 
 // Clone OldFunc into NewFunc, transforming the old arguments into references to
 // VMap values.
-//
 void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
                              ValueToValueMapTy &VMap,
-                             bool ModuleLevelChanges,
+                             CloneType CT,
                              SmallVectorImpl<ReturnInst*> &Returns,
                              const char *NameSuffix, ClonedCodeInfo *CodeInfo,
                              ValueMapTypeRemapper *TypeMapper,
                              ValueMaterializer *Materializer) {
   assert(NameSuffix && "NameSuffix cannot be null!");
 
 #ifndef NDEBUG
   for (const Argument &I : OldFunc->args())
     assert(VMap.count(&I) && "No mapping from source argument specified!");
 #endif
 
   // Copy all attributes other than those stored in the AttributeList.  We need
   // to remap the parameter indices of the AttributeList.
   AttributeList NewAttrs = NewFunc->getAttributes();
   NewFunc->copyAttributesFrom(OldFunc);
   NewFunc->setAttributes(NewAttrs);
 
+  RemapFlags RF = (CT == CloneType::ModuleLevelChanges)
+          ? RF_None : RF_NoModuleLevelChanges;
   // Fix up the personality function that got copied over.
   if (OldFunc->hasPersonalityFn())
     NewFunc->setPersonalityFn(
-        MapValue(OldFunc->getPersonalityFn(), VMap,
-                 ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+                MapValue(OldFunc->getPersonalityFn(), VMap, RF,
                  TypeMapper, Materializer));
 
   SmallVector<AttributeSet, 4> NewArgAttrs(NewFunc->arg_size());
   AttributeList OldAttrs = OldFunc->getAttributes();
 
   // Clone any argument attributes that are present in the VMap.
   for (const Argument &OldArg : OldFunc->args()) {
     if (Argument *NewArg = dyn_cast<Argument>(VMap[&OldArg])) {
       NewArgAttrs[NewArg->getArgNo()] =
           OldAttrs.getParamAttributes(OldArg.getArgNo());
     }
   }
 
   NewFunc->setAttributes(
       AttributeList::get(NewFunc->getContext(), OldAttrs.getFnAttributes(),
                          OldAttrs.getRetAttributes(), NewArgAttrs));
 
-  bool MustCloneSP =
+  bool MustCloneSP = CT != CloneType::ExtractingFunctions &&
       OldFunc->getParent() && OldFunc->getParent() == NewFunc->getParent();
   DISubprogram *SP = OldFunc->getSubprogram();
   if (SP) {
-    assert(!MustCloneSP || ModuleLevelChanges);
+    assert(!MustCloneSP || CT == CloneType::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);
   }
 
   SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
   OldFunc->getAllMetadata(MDs);
   for (auto MD : MDs) {
     NewFunc->addMetadata(
         MD.first,
-        *MapMetadata(MD.second, VMap,
-                     ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
-                     TypeMapper, Materializer));
+        *MapMetadata(MD.second, VMap, RF, TypeMapper, Materializer));
   }
 
   // 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;
#endif
   // Loop over all of the instructions in the function, fixing up operand
   // references as we go.  This uses VMap to do all the hard work.
   for (Function::iterator BB =
            cast<BasicBlock>(VMap[&OldFunc->front()])->getIterator(),
                           BE = NewFunc->end();
        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);
+      RemapInstruction(&II, VMap, RF, TypeMapper, Materializer);
 }
 
 /// 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(Function *F, ValueToValueMapTy &VMap,
   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
     }
 
   SmallVector<ReturnInst*, 8> Returns;  // Ignore returns cloned.
-  CloneFunctionInto(NewF, F, VMap, F->getSubprogram() != nullptr, Returns, "",
-                    CodeInfo);
+  CloneType CT = F->getSubprogram() != nullptr ? CloneType::ModuleLevelChanges : CloneType::InvalidCloneType;
+  CloneFunctionInto(NewF, F, VMap, CT, Returns, "", CodeInfo);
 
   return NewF;
 }
 
 
 
 namespace {
   /// This is a private class used to implement CloneAndPruneFunctionInto.

lib/Transforms/Utils/CloneModule.cpp

for (const Function &I : *M) {
     Function::arg_iterator DestI = F->arg_begin();
     for (Function::const_arg_iterator J = I.arg_begin(); J != I.arg_end();
          ++J) {
       DestI->setName(J->getName());
       VMap[&*J] = &*DestI++;
     }
 
     SmallVector<ReturnInst *, 8> Returns; // Ignore returns cloned.
-    CloneFunctionInto(F, &I, VMap, /*ModuleLevelChanges=*/true, Returns);
+    CloneFunctionInto(F, &I, VMap, CloneType::ModuleLevelChanges, Returns);
 
     if (I.hasPersonalityFn())
       F->setPersonalityFn(MapValue(I.getPersonalityFn(), VMap));
 
     copyComdat(F, &I);
   }
 
   // And aliases

test/Transforms/MergeSimilarFunc/merge-debug-info-2.ll

+; RUN: opt -S -mergesimilarfunc -mergesimilarfunc-diff-min-insts=5 < %s | FileCheck %s
+; This used to fail with assertion in CloneFunction
+; REQUIRES: asserts
+; CHECK-LABEL: @foo__merged(
+
+target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+target triple = "arm64-apple-ios8.0.0"
+
+%struct.wibble = type { %struct.wibble.0*, %struct.wibble* }
+%struct.wibble.0 = type { i64*, i8*, i64*, i64*, %struct.eggs*, %struct.wibble* }
+%struct.eggs = type { %struct.wombat*, %struct.eggs* }
+%struct.wombat = type { i8*, %struct.blam*, %struct.blam* }
+%struct.blam = type { i8*, %struct.blam* }
+%struct.snork = type { %struct.bar*, %struct.snork* }
+%struct.bar = type { i64*, i8* }
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture) #0
+
+; Function Attrs: minsize nounwind optsize ssp uwtable
+define hidden void @foo(%struct.wibble* %arg) #1 align 2 !dbg !4 {
+bb:
+  %tmp = alloca %struct.wibble*, align 8
+  %tmp2 = load %struct.wibble*, %struct.wibble** %tmp, align 8, !dbg !12
+  %tmp3 = icmp ne %struct.wibble* %tmp2, null, !dbg !13
+  br i1 %tmp3, label %bb4, label %bb13, !dbg !14
+
+bb4:                                              ; preds = %bb
+  call void @foo.1() #3, !dbg !26
+  unreachable
+
+bb13:                                             ; preds = %bb
+  ret void, !dbg !27
+}
+
+; Function Attrs: minsize nounwind optsize ssp uwtable
+declare hidden void @foo.1() #1 align 2
+
+; Function Attrs: minsize nounwind optsize ssp uwtable
+define void @quux() unnamed_addr #1 align 2 !dbg !28 {
+bb:
+  ret void
+}
+
+; Function Attrs: minsize nounwind optsize ssp uwtable
+define hidden void @baz(%struct.snork* %arg) #1 align 2 !dbg !30 {
+bb:
+  %tmp = alloca %struct.snork*, align 8
+  %tmp2 = load %struct.snork*, %struct.snork** %tmp, align 8, !dbg !31
+  %tmp3 = icmp ne %struct.snork* %tmp2, null, !dbg !32
+  br i1 %tmp3, label %bb4, label %bb13, !dbg !33
+
+bb4:                                              ; preds = %bb
+  call void @blam() #3, !dbg !42
+  unreachable
+
+bb13:                                             ; preds = %bb
+  ret void, !dbg !43
+}
+
+; Function Attrs: minsize nounwind optsize ssp uwtable
+declare hidden void @blam() #1 align 2
+
+attributes #0 = { argmemonly nounwind }
+attributes #1 = { minsize nounwind optsize ssp uwtable }
+attributes #2 = { nounwind }
+attributes #3 = { minsize optsize }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "(based on LLVM 5.0.0)", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly)
+!1 = !DIFile(filename: "foo.cpp", directory: "/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !{i32 7, !"PIC Level", i32 2}
+!4 = distinct !DISubprogram(name: "delete", scope: !5, file: !5, line: 31, type: !6, isLocal: false, isDefinition: true, scopeLine: 32, flags: DIFlagPrototyped, isOptimized: true, unit: !0)
+!5 = !DIFile(filename: "foo.h", directory: "/")
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!9 = !{!"any pointer", !10, i64 0}
+!10 = !{!"omnipotent char", !11, i64 0}
+!11 = !{!"Simple C++ TBAA"}
+!12 = !DILocation(line: 33, column: 11, scope: !4)
+!13 = !DILocation(line: 33, column: 16, scope: !4)
+!14 = !DILocation(line: 33, column: 5, scope: !4)
+!21 = !{!"_ZTSN", !9, i64 0, !9, i64 8}
+!23 = !DILocation(line: 37, column: 25, scope: !4)
+!24 = !DILocation(line: 37, column: 31, scope: !4)
+!25 = !{!21, !9, i64 0}
+!26 = !DILocation(line: 37, column: 7, scope: !4)
+!27 = !DILocation(line: 41, column: 3, scope: !4)
+!28 = distinct !DISubprogram(name: "~destruct", scope: !29, file: !29, line: 31, type: !6, isLocal: false, isDefinition: true, scopeLine: 32, flags: DIFlagPrototyped, isOptimized: true, unit: !0)
+!29 = !DIFile(filename: "bar.h", directory: "/")
+!30 = distinct !DISubprogram(name: "delete", scope: !5, file: !5, line: 31, type: !6, isLocal: false, isDefinition: true, scopeLine: 32, flags: DIFlagPrototyped, isOptimized: true, unit: !0)
+!31 = !DILocation(line: 33, column: 11, scope: !30)
+!32 = !DILocation(line: 33, column: 16, scope: !30)
+!33 = !DILocation(line: 33, column: 5, scope: !30)
+!40 = !DILocation(line: 37, column: 25, scope: !30)
+!41 = !DILocation(line: 37, column: 31, scope: !30)
+!42 = !DILocation(line: 37, column: 7, scope: !30)
+!43 = !DILocation(line: 41, column: 3, scope: !30)

unittests/Transforms/Utils/Cloning.cpp

TEST_F(CloneInstruction, Attributes) {
 
   Argument *A = &*F1->arg_begin();
   A->addAttr(Attribute::NoCapture);
 
   SmallVector<ReturnInst*, 4> Returns;
   ValueToValueMapTy VMap;
   VMap[A] = UndefValue::get(A->getType());
 
-  CloneFunctionInto(F2, F1, VMap, false, Returns);
+  CloneFunctionInto(F2, F1, VMap, CloneType::NoModuleLevelChanges, Returns);
   EXPECT_FALSE(F2->arg_begin()->hasNoCaptureAttr());
 
   delete F1;
   delete F2;
 }
 
 TEST_F(CloneInstruction, CallingConvention) {
   Type *ArgTy1[] = { Type::getInt32PtrTy(context) };
   FunctionType *FT1 =  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
 
   Function *F1 = Function::Create(FT1, Function::ExternalLinkage);
   F1->setCallingConv(CallingConv::Cold);
   BasicBlock *BB = BasicBlock::Create(context, "", F1);
   IRBuilder<> Builder(BB);
   Builder.CreateRetVoid();
 
   Function *F2 = Function::Create(FT1, Function::ExternalLinkage);
 
   SmallVector<ReturnInst*, 4> Returns;
   ValueToValueMapTy VMap;
   VMap[&*F1->arg_begin()] = &*F2->arg_begin();
 
-  CloneFunctionInto(F2, F1, VMap, false, Returns);
+  CloneFunctionInto(F2, F1, VMap, CloneType::NoModuleLevelChanges, Returns);
   EXPECT_EQ(CallingConv::Cold, F2->getCallingConv());
 
   delete F1;
   delete F2;
 }
 
 TEST_F(CloneInstruction, DuplicateInstructionsToSplit) {
   Type *ArgTy1[] = {Type::getInt32PtrTy(context)};