[SplitModule] Do not copy stale debug info during module splitting.

[slarin]

Summary

Currently CloneModule always copy debug related metadata even if debugging subject was not cloned
in the target module. This is not only suboptimal in size and time, but can potentially create
issues when metadata retains references to objects that were not cloned in the target module.

This patch attempts bluntly address the effect without fixing the cause.

[slarin]

Guys, can you please take a quick look...?

Thanks...

Sergei

[mehdi_amini]

Sorry for the delay, I'm not a debug info expert, so I won't be able to catch all corner cases, maybe D.Blakie or A.Prantl can help?
On the approach itself, it seems like a band-aid compared to filtering out what should be imported in the first place. Maybe Teresa who worked on the filtering has some input (or better: some refactored code to share?).

lib/Transforms/Utils/SplitModule.cpp

223

Nit: F is used elsewhere for Function, rename?

258

Not sure if it is the right thing to do, but I'm gonna leave this to people more aware of debug info than I am.

[tejohnson]

In D17338#365399, @joker.eph wrote:

Sorry for the delay, I'm not a debug info expert, so I won't be able to catch all corner cases, maybe D.Blakie or A.Prantl can help?
On the approach itself, it seems like a band-aid compared to filtering out what should be imported in the first place. Maybe Teresa who worked on the filtering has some input (or better: some refactored code to share?).

I agree with Mehdi that it would be ideal to fix this in CloneModule itself. And perhaps it can leverage the changes I have in D16440 which additionally avoids bringing in unnecessary composite types. Looks like it would be fairly natural as they both use a ValueMap and so the identification of what is needed by the mapped in functions should be the same. The support in my patch would need to be moved out to a common place though.

To get CloneModule to do the right thing with regards to subprogram metadata like you are doing here, AFAICT it will need to fix the way it is currently mapping subprograms (see the comment I just left on D17165).

lib/Transforms/Utils/SplitModule.cpp

 
 #define DEBUG_TYPE "split-module"
 
 #include "llvm/Transforms/Utils/SplitModule.h"
 #include "llvm/ADT/EquivalenceClasses.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalObject.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/MD5.h"
static bool isInPartition(const GlobalValue *GV, unsigned I, unsigned N) {
   // are enough.
   MD5 H;
   MD5::MD5Result R;
   H.update(Name);
   H.final(R);
   return (R[0] | (R[1] << 8)) % N == I;
 }
 
+/// Remove any debug info for global variables/functions in the given module for
+/// which said global variable/function is not in the same partition.
+/// This is very similar, but fundamentally non-identical to StripDeadDebugInfo
+/// pass.
+static void StripDeadDebugInfo(Module &M) {
+  DebugInfoFinder F;
+  F.processModule(M);
+
+  // For each compile unit, find the live set of global variables/functions and
+  // replace the current list of potentially dead global variables/functions
+  // with the live list.
+  SmallVector<Metadata *, 64> LiveGlobalVariables;
+  SmallVector<Metadata *, 64> LiveSubprograms;
+  DenseSet<const MDNode *> VisitedSet;
+
+  std::set<DISubprogram *> LiveSPs;
+  for (Function &F : M) {
+    if (DISubprogram *SP = F.getSubprogram())
+      LiveSPs.insert(SP);
+  }
+
+  for (DICompileUnit *DIC : F.compile_units()) {
+    bool SubprogramChange = false;
+    for (DISubprogram *DISP : DIC->getSubprograms()) {
+      if (!VisitedSet.insert(DISP).second)
+        continue;
+
+      // If the function referenced by DISP is not null, the function is live.
+      if (LiveSPs.count(DISP))
+        LiveSubprograms.push_back(DISP);
+      else
+        SubprogramChange = true;
+    }
+
+    bool GlobalVariableChange = false;
+    for (DIGlobalVariable *DIG : DIC->getGlobalVariables()) {
+      if (!VisitedSet.insert(DIG).second)
+        continue;
+      // This is the principal difference... Instead of null, the
+      // global variable that ended up in another partition will be a
+      // declaration.
+      if (DIG->getVariable()) {
+        GlobalValue *GV = dyn_cast<GlobalValue>(DIG->getVariable());
+        if (GV && !GV->isDeclaration()) {
+          LiveGlobalVariables.push_back(DIG);
+          continue;
+        }
+      }
+      GlobalVariableChange = true;
+    }
+
+    // If we found dead subprograms or global variables, replace the current
+    // subprogram list/global variable list with our new live subprogram/global
+    // variable list.
+    if (SubprogramChange)
+      DIC->replaceSubprograms(MDTuple::get(M.getContext(), LiveSubprograms));
+
+    if (GlobalVariableChange)
+      DIC->replaceGlobalVariables(
+          MDTuple::get(M.getContext(), LiveGlobalVariables));
+
+    LiveSubprograms.clear();
+    LiveGlobalVariables.clear();
+  }
+}
+
 void llvm::SplitModule(
     std::unique_ptr<Module> M, unsigned N,
     std::function<void(std::unique_ptr<Module> MPart)> ModuleCallback,
     bool PreserveLocals) {
   if (!PreserveLocals) {
     for (Function &F : *M)
       externalize(&F);
     for (GlobalVariable &GV : M->globals())
for (unsigned I = 0; I < N; ++I) {
       ValueToValueMapTy VMap;
       std::unique_ptr<Module> MPart(
           CloneModule(M.get(), VMap, [&](const GlobalValue *GV) {
              if (ClusterIDMap.count(GV))
                return (ClusterIDMap[GV] == I);
              else
                return isInPartition(GV, I, N);
           }));
+      // Clean up unused metadata.
+      StripDeadDebugInfo(*MPart.get());
+
       if (I != 0)
         MPart->setModuleInlineAsm("");
       ModuleCallback(std::move(MPart));
     }
   }
 }

test/tools/llvm-split/test_split_debug.ll

+; RUN: llvm-split -j=2 -preserve-locals -o %t %s
+; RUN: llvm-dis -o - %t0 | FileCheck --check-prefix=CHECK0 %s
+; RUN: llvm-dis -o - %t1 | FileCheck --check-prefix=CHECK1 %s
+
+; Make sure we do not copy debug metadata when it is not needed.
+; CHECK0: DISubprogram(name: "foo"
+; CHECK0: DISubprogram(name: "local_func"
+; CHECK1-NOT: DISubprogram(name: "foo"
+; CHECK1-NOT: DISubprogram(name: "local_func"
+
+@global_storage = external global i32, align 4
+@local_var = internal global i32 0, align 4
+
+; Function Attrs: noinline nounwind
+define i32 @foo(i32 %a, i32* %b) #0 !dbg !4 {
+entry:
+  %a.addr = alloca i32, align 4
+  %b.addr = alloca i32*, align 4
+  store i32 %a, i32* %a.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %a.addr, metadata !25, metadata !26), !dbg !27
+  store i32* %b, i32** %b.addr, align 4
+  call void @llvm.dbg.declare(metadata i32** %b.addr, metadata !28, metadata !26), !dbg !29
+  call void @local_func(), !dbg !30
+  %0 = load i32, i32* %a.addr, align 4, !dbg !31
+  %call = call i32 @bar(i32 %0), !dbg !32
+  %1 = load i32, i32* @global_storage, align 4, !dbg !33
+  %call1 = call i32 @baz(i32 %1), !dbg !34
+  %add = add nsw i32 %call, %call1, !dbg !36
+  %2 = load i32*, i32** %b.addr, align 4, !dbg !37
+  store i32 %add, i32* %2, align 4, !dbg !38
+  %3 = load i32*, i32** %b.addr, align 4, !dbg !39
+  %4 = load i32, i32* %3, align 4, !dbg !40
+  %call2 = call i32 @car(i32 %4), !dbg !41
+  %5 = load volatile i32, i32* @local_var, align 4, !dbg !42
+  %add3 = add nsw i32 %call2, %5, !dbg !43
+  ret i32 %add3, !dbg !44
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare i32 @bar(i32) #2
+
+declare i32 @baz(i32) #2
+
+declare i32 @car(i32) #2
+
+; Function Attrs: noinline nounwind
+define internal void @local_func() #0 !dbg !9 {
+entry:
+  %0 = load i32, i32* @global_storage, align 4, !dbg !45
+  %call = call i32 @foo(i32 %0, i32* null), !dbg !46
+  store volatile i32 %call, i32* @local_var, align 4, !dbg !47
+  ret void, !dbg !48
+}
+
+attributes #0 = { noinline nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="hexagonv5" "target-features"="-hvx,-hvx-double" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone }
+attributes #2 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="hexagonv5" "target-features"="-hvx,-hvx-double" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!15, !16, !17, !19}
+!llvm.ident = !{!21}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "Clang (based on LLVM 3.9.0)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3, globals: !12)
+!1 = !DIFile(filename: "split.c", directory: "/local/mnt/workspace")
+!2 = !{}
+!3 = !{!4, !9}
+!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 12, type: !5, isLocal: false, isDefinition: true, scopeLine: 12, flags: DIFlagPrototyped, isOptimized: false, variables: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{!7, !7, !8}
+!7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 32, align: 32)
+!9 = distinct !DISubprogram(name: "local_func", scope: !1, file: !1, line: 8, type: !10, isLocal: true, isDefinition: true, scopeLine: 8, isOptimized: false, variables: !2)
+!10 = !DISubroutineType(types: !11)
+!11 = !{null}
+!12 = !{!13}
+!13 = !DIGlobalVariable(name: "local_var", scope: !0, file: !1, line: 6, type: !14, isLocal: true, isDefinition: true, variable: i32* @local_var)
+!14 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !7)
+!15 = !{i32 2, !"Dwarf Version", i32 4}
+!16 = !{i32 2, !"Debug Info Version", i32 3}
+!17 = !{i32 6, !"Target CPU", !18}
+!18 = !{!"hexagonv5"}
+!19 = !{i32 6, !"Target Features", !20}
+!20 = !{!"-Os", !"-double"}
+!21 = !{!"Clang (based on LLVM 3.9.0)"}
+!25 = !DILocalVariable(name: "a", arg: 1, scope: !4, file: !1, line: 12, type: !7)
+!26 = !DIExpression()
+!27 = !DILocation(line: 12, column: 39, scope: !4)
+!28 = !DILocalVariable(name: "b", arg: 2, scope: !4, file: !1, line: 12, type: !8)
+!29 = !DILocation(line: 12, column: 47, scope: !4)
+!30 = !DILocation(line: 13, column: 3, scope: !4)
+!31 = !DILocation(line: 14, column: 12, scope: !4)
+!32 = !DILocation(line: 14, column: 8, scope: !4)
+!33 = !DILocation(line: 14, column: 19, scope: !4)
+!34 = !DILocation(line: 14, column: 15, scope: !35)
+!35 = !DILexicalBlockFile(scope: !4, file: !1, discriminator: 1)
+!36 = !DILocation(line: 14, column: 14, scope: !4)
+!37 = !DILocation(line: 14, column: 4, scope: !4)
+!38 = !DILocation(line: 14, column: 6, scope: !4)
+!39 = !DILocation(line: 15, column: 13, scope: !4)
+!40 = !DILocation(line: 15, column: 12, scope: !4)
+!41 = !DILocation(line: 15, column: 8, scope: !4)
+!42 = !DILocation(line: 15, column: 18, scope: !4)
+!43 = !DILocation(line: 15, column: 16, scope: !4)
+!44 = !DILocation(line: 15, column: 1, scope: !4)
+!45 = !DILocation(line: 9, column: 19, scope: !9)
+!46 = !DILocation(line: 9, column: 15, scope: !9)
+!47 = !DILocation(line: 9, column: 13, scope: !9)
+!48 = !DILocation(line: 10, column: 1, scope: !9)