[cloning] Do not duplicate types when cloning functions

[GorNishanov]

Summary

This is an addon to the change rl304488 cloning fixes. (Originally rl304226 reverted rl304228 and reapplied rl304488 https://reviews.llvm.org/D33655)

rl304488 works great when DILocalVariables that comes from the inlined function has a 'unique-ed' type, but,
in the case when the variable type is distinct we will create a second DILocalVariable in the scope of the original function that was inlined.

Consider cloning of the following function:

define private void @f() !dbg !5 {
  %1 = alloca i32, !dbg !11
  call void @llvm.dbg.declare(metadata i32* %1, metadata !14, metadata !12), !dbg !18
  ret void, !dbg !18
}

!14 = !DILocalVariable(name: "inlined", scope: !15, file: !6, line: 5, type: !17) ; came from an inlined function
!15 = distinct !DISubprogram(name: "inlined", linkageName: "inlined", scope: null, file: !6, line: 8, type: !7, isLocal: true, isDefinition: true, scopeLine: 9, isOptimized: false, unit: !0, variables: !16)
!16 = !{!14}
!17 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "some_struct", size: 32, align: 32)

Without this fix, when function 'f' is cloned, we will create another DILocalVariable for "inlined", due to its type being distinct.

define private void @f.1() !dbg !23 {
  %1 = alloca i32, !dbg !26
  call void @llvm.dbg.declare(metadata i32* %1, metadata !28, metadata !12), !dbg !30
  ret void, !dbg !30
}

!14 = !DILocalVariable(name: "inlined", scope: !15, file: !6, line: 5, type: !17)
!15 = distinct !DISubprogram(name: "inlined", linkageName: "inlined", scope: null, file: !6, line: 8, type: !7, isLocal: true, isDefinition: true, scopeLine: 9, isOptimized: false, unit: !0, variables: !16)
!16 = !{!14}
!17 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "some_struct", size: 32, align: 32)
 ; 
!28 = !DILocalVariable(name: "inlined", scope: !15, file: !6, line: 5, type: !29) ; OOPS second DILocalVariable
!29 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "some_struct", size: 32, align: 32)

Now we have two DILocalVariable for "inlined" within the same scope. This result in assert in AsmPrinter/DwarfDebug.h:131: void llvm::DbgVariable::addMMIEntry(const llvm::DbgVariable &): Assertion `V.Var == Var && "conflicting variable"' failed.
(Full example: See: https://bugs.llvm.org/show_bug.cgi?id=33492)

In this change we prevent duplication of types so that when a metadata for DILocalVariable is cloned it will get uniqued to the same metadate node as an original variable.

[loladiro]

The implementation of this looks good to me. I guess I'm not entirely sure why it's necessary for that type to be distinct in the first place, but that's probably a different discussion from just fixing this bug.

lib/Transforms/Utils/CloneFunction.cpp

49

Does this ever get called without a valid F?

[GorNishanov]

In D35106#801808, @loladiro wrote:

The implementation of this looks good to me. I guess I'm not entirely sure why it's necessary for that type to be distinct in the first place, but that's probably a different discussion from just fixing this bug.

Clang code has a comment that:

// Elements of composite types usually have back to the type, creating
// uniquing cycles.  Distinct nodes are more efficient.

lib/Transforms/Utils/CloneFunction.cpp

49

Yes, In two places.
In llvm/lib/CodeGen/WinEHPrepare.cpp and
in llvm/lib/Transforms/Utils/LoopUnroll.cpp

I am wondering if it is just an oversight due to the F parameter having a default value of nullptr and in those two places function parameter is omitted.

[GorNishanov]

Thank you for the review. Preparing to land.

lib/Transforms/Utils/CloneFunction.cpp

BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
                                   const Twine &NameSuffix, Function *F,
                                   ClonedCodeInfo *CodeInfo,
                                   DebugInfoFinder *DIFinder) {
   DenseMap<const MDNode *, MDNode *> Cache;
   BasicBlock *NewBB = BasicBlock::Create(BB->getContext(), "", F);
   if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);
 
   bool hasCalls = false, hasDynamicAllocas = false, hasStaticAllocas = false;
-  
+  Module *TheModule = F ? F->getParent() : nullptr;
   // Loop over all instructions, and copy them over.
   for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
        II != IE; ++II) {
 
-    if (DIFinder && F->getParent() && II->getDebugLoc())
-      DIFinder->processLocation(*F->getParent(), II->getDebugLoc().get());
+    if (DIFinder && TheModule) {
+      if (auto *DDI = dyn_cast<DbgDeclareInst>(II))
+        DIFinder->processDeclare(*TheModule, DDI);
+      else if (auto *DVI = dyn_cast<DbgValueInst>(II))
+        DIFinder->processValue(*TheModule, DVI);
+
+      if (auto DbgLoc = II->getDebugLoc())
+        DIFinder->processLocation(*TheModule, DbgLoc.get());
+    }
 
     Instruction *NewInst = II->clone();
     if (II->hasName())
       NewInst->setName(II->getName()+NameSuffix);
     NewBB->getInstList().push_back(NewInst);
     VMap[&*II] = NewInst; // Add instruction map to value.
 
     hasCalls |= (isa<CallInst>(II) && !isa<DbgInfoIntrinsic>(II));
NewFunc->addMetadata(
         *MapMetadata(MD.second, VMap,
                      ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
                      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;
 
   // 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.
   //
   for (Function::const_iterator BI = OldFunc->begin(), BE = OldFunc->end();
        BI != BE; ++BI) {
#endif
   }
 
   for (DISubprogram *ISP : DIFinder.subprograms()) {
     if (ISP != SP) {
       VMap.MD()[ISP].reset(ISP);
     }
   }
 
+  for (auto *Type : DIFinder.types()) {
+    VMap.MD()[Type].reset(Type);
+  }
+
   // 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)

unittests/Transforms/Utils/Cloning.cpp

void CreateOldFunctionBodyAndDI() {
     auto *E = DBuilder.createExpression();
     auto *Variable =
         DBuilder.createAutoVariable(Subprogram, "x", File, 5, IntType, true);
     auto *DL = DILocation::get(Subprogram->getContext(), 5, 0, Subprogram);
     DBuilder.insertDeclare(Alloca, Variable, E, DL, Store);
     DBuilder.insertDbgValueIntrinsic(AllocaContent, 0, Variable, E, DL,
                                      Entry);
     // Also create an inlined variable.
+    // Create a distinct struct type that we should not duplicate during
+    // cloning).
+    auto *StructType = DICompositeType::getDistinct(
+        C, dwarf::DW_TAG_structure_type, "some_struct", nullptr, 0, nullptr,
+        nullptr, 32, 32, 0, DINode::FlagZero, nullptr, 0, nullptr, nullptr);
     auto *InlinedSP =
         DBuilder.createFunction(CU, "inlined", "inlined", File, 8, FuncType,
                                 true, true, 9, DINode::FlagZero, false);
     auto *InlinedVar =
-        DBuilder.createAutoVariable(InlinedSP, "inlined", File, 5, IntType, true);
+        DBuilder.createAutoVariable(InlinedSP, "inlined", File, 5, StructType, true);
     auto *Scope = DBuilder.createLexicalBlock(
         DBuilder.createLexicalBlockFile(InlinedSP, File), File, 1, 1);
     auto InlinedDL =
         DebugLoc::get(9, 4, Scope, DebugLoc::get(5, 2, Subprogram));
     IBuilder.SetCurrentDebugLocation(InlinedDL);
     DBuilder.insertDeclare(Alloca, InlinedVar, E, InlinedDL, Store);
     IBuilder.CreateStore(IBuilder.getInt32(2), Alloca);
     // Finalize the debug info.
if (DbgDeclareInst* OldIntrin = dyn_cast<DbgDeclareInst>(&OldI)) {
 
       // Old address must belong to the old function
       EXPECT_EQ(OldFunc, cast<AllocaInst>(OldIntrin->getAddress())->
                          getParent()->getParent());
       // New address must belong to the new function
       EXPECT_EQ(NewFunc, cast<AllocaInst>(NewIntrin->getAddress())->
                          getParent()->getParent());
 
-      if (!OldIntrin->getDebugLoc()->getInlinedAt()) {
+      if (OldIntrin->getDebugLoc()->getInlinedAt()) {
+        // Inlined variable should refer to the same DILocalVariable as in the
+        // Old Function
+        EXPECT_EQ(OldIntrin->getVariable(), NewIntrin->getVariable());
+      } else {
         // Old variable must belong to the old function.
         EXPECT_EQ(OldFunc->getSubprogram(),
                   cast<DISubprogram>(OldIntrin->getVariable()->getScope()));
         // New variable must belong to the new function.
         EXPECT_EQ(NewFunc->getSubprogram(),
                   cast<DISubprogram>(NewIntrin->getVariable()->getScope()));
       }
     } else if (DbgValueInst* OldIntrin = dyn_cast<DbgValueInst>(&OldI)) {