Add optional DICompileUnit to DIBuilder and make outlined debug info use that CU

[paquette]

Summary

Previously, I committed a change to the outliner which added some debug information to outlined functions. This wasn't entirely great because DIBuilder only sets its CU using createCompileUnit. As a result, the outliner had to make a second compile unit just for the outlined sequences.

This sparked an e-mail discussion on the topic, so I'm putting the patch up here just because it's a bit easier to read + discuss.

The only change to DIBuilder is an added optional parameter in the constructor which allows you to set the CU if you already know what you want. There are no functional changes to it otherwise. The rest of the changes are in the outliner. It works like this:

1. Keep track of the MachineFunction that each Candidate appears in. Use that to get a CU for that Candidate if possible.
2. To find a CU for an OutlinedFunction, grab the first CU belonging to some Candidate associated with that function.
3. When you create a function, see if it has a potential CU. If it does, then generate debug info for the outlined function.

[aprantl]

lib/CodeGen/MachineOutliner.cpp

116

How about getCompileUnitOrNull?

179

Nitpick: there is no need to use \brief in Doxygen comments. LLVM uses the autobrief option.

181

perhaps shorter:

for (auto &C : Candidates) 
  if (auto *CU = C->getCUIfExists())
    return CU;
 return nullptr;

1282

Why not just allocate the DIBuilder on the stack?
DIBuilder DB(M, true, CU)

1283

Can you also grab the file from the DISubprogram that you got the CU from?

1290

I'd also put the linkage name here.

1291

0 /* Line 0 is reserved for compiler-generated code */

1292

rest looks good!

[dblaikie]

lib/CodeGen/MachineOutliner.cpp

116

Maybe "OrNull" rather than "IfExists". Maybe check if one's more prevalent than the other across the LLVM codebase - but either's fine, really.

182–188

I'd probably write that as:

for (const auto &C  : Candidates)
  if (auto *CU = C->getCUIfExists())
    return CU;
return nullptr;

1288–1295

Any chance of cloning the DISubprogram from the original function, then modifying it? Or does it seem better to create a separate one like this? I guess this is fine (Adrian?)

[aprantl]

lib/CodeGen/MachineOutliner.cpp

1288–1295

Since it will be a distinct MDNode modifying the clone is technically legal, but I don't consider it good practice. I think this approach is cleaner and easier to understand.

[paquette]

Updated diff to reflect style notes and address review comments.

[aprantl]

lib/CodeGen/MachineOutliner.cpp

1283

Looks like this one was accidentally marked as done?

[paquette]

lib/CodeGen/MachineOutliner.cpp

1283

It's CU->getFile() on here now but in the e-mail thread, it appears to still be

DIFile *Unit = DB->createFile(CU->getName(), CU->getDirectory())

Maybe that got mixed up somehow? Or is that still incorrect? Is it possible for a DISubprogram to have a different DIFile than its CU?

[aprantl]

lib/CodeGen/MachineOutliner.cpp

1283

Is it possible for a DISubprogram to have a different DIFile than its CU?

Yes. For example, two functions defined in different header files have different files even if they belong to the same CU.

My original suggestion was to use SP->getFile() here where SP is the DISubprogram attached to the function we got the CU from.

That said, if we are outlining code from two function from two different files, this file will be wrong and I'm not sure how to handle this. Should we create a new "virtual" file "<outlined-code>"? What do others think?

[probinson]

lib/CodeGen/MachineOutliner.cpp

1283

I think taking the DIFile from the same DISubprogram where we got the CU is fine. It's more consistent than anything else we could do.

[paquette]

Updated diff so that the file comes from the subprogram instead of the compile unit.

[paquette]

Since it seems like everyone's concerns have been cleared up, I'm going to go ahead and commit this.

Thanks to everyone for their input! If there are any other concerns let me know.

include/llvm/IR/DIBuilder.h

insertDbgValueIntrinsic(llvm::Value *Val, DILocalVariable *VarInfo,
                             DIExpression *Expr, const DILocation *DL,
                             BasicBlock *InsertBB, Instruction *InsertBefore);
 
   public:
     /// Construct a builder for a module.
     ///
     /// If \c AllowUnresolved, collect unresolved nodes attached to the module
     /// in order to resolve cycles during \a finalize().
-    explicit DIBuilder(Module &M, bool AllowUnresolved = true);
+    ///
+    /// If \p CU is given a value other than nullptr, then set \p CUNode to CU.
+    explicit DIBuilder(Module &M, bool AllowUnresolved = true,
+                       DICompileUnit *CU = nullptr);
     DIBuilder(const DIBuilder &) = delete;
     DIBuilder &operator=(const DIBuilder &) = delete;
 
     /// Construct any deferred debug info descriptors.
     void finalize();
 
     /// Finalize a specific subprogram - no new variables may be added to this
     /// subprogram afterwards.

lib/CodeGen/MachineOutliner.cpp

 struct Candidate {
 private:
   /// The start index of this \p Candidate in the instruction list.
   unsigned StartIdx;
 
   /// The number of instructions in this \p Candidate.
   unsigned Len;
 
+  /// The MachineFunction containing this \p Candidate.
+  MachineFunction *MF = nullptr;
+
 public:
   /// Set to false if the candidate overlapped with another candidate.
   bool InCandidateList = true;
 
   /// \brief The index of this \p Candidate's \p OutlinedFunction in the list of
   /// \p OutlinedFunctions.
   unsigned FunctionIdx;
 
   /// Contains all target-specific information for this \p Candidate.
   TargetInstrInfo::MachineOutlinerInfo MInfo;
 
+  /// If there is a DICompileUnit associated with the function that this
+  /// \p Candidate lives in, return it.
+  DICompileUnit *getCUIfExists() const {
+    assert(MF && "Candidate has no MF!");
+    if (DISubprogram *SP = MF->getFunction().getSubprogram())
+      return SP->getUnit();
+    return nullptr;
+  }
+
   /// Return the number of instructions in this Candidate.
   unsigned getLength() const { return Len; }
 
   /// Return the start index of this candidate.
   unsigned getStartIdx() const { return StartIdx; }
 
   // Return the end index of this candidate.
   unsigned getEndIdx() const { return StartIdx + Len - 1; }
 
   /// \brief The number of instructions that would be saved by outlining every
   /// candidate of this type.
   ///
   /// This is a fixed value which is not updated during the candidate pruning
   /// process. It is only used for deciding which candidate to keep if two
   /// candidates overlap. The true benefit is stored in the OutlinedFunction
   /// for some given candidate.
   unsigned Benefit = 0;
 
-  Candidate(unsigned StartIdx, unsigned Len, unsigned FunctionIdx)
-      : StartIdx(StartIdx), Len(Len), FunctionIdx(FunctionIdx) {}
+  Candidate(unsigned StartIdx, unsigned Len, unsigned FunctionIdx,
+            MachineFunction *MF)
+      : StartIdx(StartIdx), Len(Len), MF(MF), FunctionIdx(FunctionIdx) {}
 
   Candidate() {}
 
   /// \brief Used to ensure that \p Candidates are outlined in an order that
   /// preserves the start and end indices of other \p Candidates.
   bool operator<(const Candidate &RHS) const {
     return getStartIdx() > RHS.getStartIdx();
   }
public:
 
   /// \brief The sequence of integers corresponding to the instructions in this
   /// function.
   std::vector<unsigned> Sequence;
 
   /// Contains all target-specific information for this \p OutlinedFunction.
   TargetInstrInfo::MachineOutlinerInfo MInfo;
 
+  /// \brief If there is a DICompileUnit for any Candidate for this outlined
+  /// function, then return it. Otherwise, return nullptr.
+  DICompileUnit *getCUIfExists() const {
+    DICompileUnit *CU = nullptr;
+    for (auto &C : Candidates) {
+      CU = C->getCUIfExists();
+      if (CU)
+        break;
+    }
+    return CU;
+  }
+
   /// Return the number of candidates for this \p OutlinedFunction.
   unsigned getOccurrenceCount() { return OccurrenceCount; }
 
   /// Decrement the occurrence count of this OutlinedFunction and return the
   /// new count.
   unsigned decrement() {
     assert(OccurrenceCount > 0 && "Can't decrement an empty function!");
     OccurrenceCount--;
for (auto &ChildPair : Parent.Children) {
                           return (EndIdx < C.getStartIdx() ||
                                   StartIdx > C.getEndIdx()); 
                         })) {
           // It doesn't overlap with anything, so we can outline it.
           // Each sequence is over [StartIt, EndIt].
           MachineBasicBlock::iterator StartIt = Mapper.InstrList[StartIdx];
           MachineBasicBlock::iterator EndIt = Mapper.InstrList[EndIdx];
 
+          // Save the MachineFunction containing the Candidate.
+          MachineFunction *MF = StartIt->getParent()->getParent();
+          assert(MF && "Candidate doesn't have a MF?");
+
           // Save the candidate and its location.
           CandidatesForRepeatedSeq.emplace_back(StartIdx, StringLen,
-                                                FunctionList.size());
+                                                FunctionList.size(), MF);
           RepeatedSequenceLocs.emplace_back(std::make_pair(StartIt, EndIt));
         }
       }
     }
 
     // We've found something we might want to outline.
     // Create an OutlinedFunction to store it and check if it'd be beneficial
     // to outline.
for (unsigned Str : OF.Sequence) {
 
     // Don't keep debug information for outlined instructions.
     NewMI->setDebugLoc(DebugLoc());
     MBB.insert(MBB.end(), NewMI);
   }
 
   TII.insertOutlinerEpilogue(MBB, MF, OF.MInfo);
 
+  // If there's a compile unit associated with this outlined function, then
+  // emit debug info for the outlined function.
+  if (DICompileUnit *CU = OF.getCUIfExists()) {
+
+    std::unique_ptr<DIBuilder> DB = llvm::make_unique<DIBuilder>(M, true, CU);
+    DIFile *Unit = DB->createFile(CU->getName(), CU->getDirectory());
+
+    // Walk over each IR function we created in the outliner and create
+    // DISubprograms for each function.
+    for (Function *F : CreatedIRFunctions) {
+      DISubprogram *SP = DB->createFunction(
+          Unit /* Context */, F->getName(),
+          StringRef() /* Empty linkage name. */, Unit /* File */,
+          0 /* Line numbers don't matter*/,
+          DB->createSubroutineType(DB->getOrCreateTypeArray(None)), /* void */
+          false, true, 0, /* Line in scope doesn't matter*/
+          DINode::DIFlags::FlagArtificial /* Compiler-generated code. */,
+          true /* Outlined code is optimized code by definition. */);
+
+      // Don't add any new variables to the subprogram.
+      DB->finalizeSubprogram(SP);
+
+      // Attach subprogram to the function.
+      F->setSubprogram(SP);
+    }
+
+    // We're done with the DIBuilder.
+    DB->finalize();
+  }
+
   return &MF;
 }
 
 bool MachineOutliner::outline(
     Module &M, const ArrayRef<std::shared_ptr<Candidate>> &CandidateList,
     std::vector<OutlinedFunction> &FunctionList, InstructionMapper &Mapper) {
 
   bool OutlinedSomething = false;
unsigned MaxCandidateLen =
       buildCandidateList(CandidateList, FunctionList, ST, Mapper, *TII);
 
   // Remove candidates that overlap with other candidates.
   pruneOverlaps(CandidateList, FunctionList, Mapper, MaxCandidateLen, *TII);
 
   // Outline each of the candidates and return true if something was outlined.
   bool OutlinedSomething = outline(M, CandidateList, FunctionList, Mapper);
 
-  // If we have a compile unit, and we've outlined something, then set debug
-  // information on the outlined function.
-  if (M.debug_compile_units_begin() != M.debug_compile_units_end() &&
-      OutlinedSomething) {
-    std::unique_ptr<DIBuilder> DB = llvm::make_unique<DIBuilder>(M);
-
-    // Create a compile unit for the outlined function.
-    DICompileUnit *MCU = *M.debug_compile_units_begin();
-    DIFile *Unit = DB->createFile(M.getName(), "/");
-    DB->createCompileUnit(MCU->getSourceLanguage(), Unit, "machine-outliner",
-                          true, "", MCU->getRuntimeVersion(), StringRef(),
-                          DICompileUnit::DebugEmissionKind::NoDebug);
-
-    // Walk over each IR function we created in the outliner and create
-    // DISubprograms for each function.
-    for (Function *F : CreatedIRFunctions) {
-      DISubprogram *SP = DB->createFunction(
-          Unit /* Context */, F->getName(),
-          StringRef() /* Empty linkage name. */, Unit /* File */,
-          0 /* Line numbers don't matter*/,
-          DB->createSubroutineType(DB->getOrCreateTypeArray(None)), /* void */
-          false, true, 0, /* Line in scope doesn't matter*/
-          DINode::DIFlags::FlagArtificial /* Compiler-generated code. */,
-          true /* Outlined code is optimized code by definition. */);
-
-      // Don't add any new variables to the subprogram.
-      DB->finalizeSubprogram(SP);
-
-      // Attach subprogram to the function.
-      F->setSubprogram(SP);
-    }
-
-    // We're done with the DIBuilder.
-    DB->finalize();
-  }
-
   return OutlinedSomething;
 }

lib/IR/DIBuilder.cpp

 using namespace llvm;
 using namespace llvm::dwarf;
 
 cl::opt<bool>
     UseDbgAddr("use-dbg-addr",
                llvm::cl::desc("Use llvm.dbg.addr for all local variables"),
                cl::init(false), cl::Hidden);
 
-DIBuilder::DIBuilder(Module &m, bool AllowUnresolvedNodes)
-  : M(m), VMContext(M.getContext()), CUNode(nullptr),
+DIBuilder::DIBuilder(Module &m, bool AllowUnresolvedNodes, DICompileUnit *CU)
+  : M(m), VMContext(M.getContext()), CUNode(CU),
       DeclareFn(nullptr), ValueFn(nullptr),
       AllowUnresolvedNodes(AllowUnresolvedNodes) {}
 
 void DIBuilder::trackIfUnresolved(MDNode *N) {
   if (!N)
     return;
   if (N->isResolved())
     return;

test/CodeGen/X86/machine-outliner-disubprogram.ll

entry:
   call void @llvm.dbg.declare(metadata i32* %a, metadata !68, metadata !DIExpression()), !dbg !69
   store i32 4, i32* %a, align 4, !dbg !69
   %call = call i32 @f1() #2, !dbg !70
   %call1 = call i32 @f2() #2, !dbg !71
   %call2 = call i32 @f3() #2, !dbg !72
   ret i32 0, !dbg !73
 }
 
-; CHECK [[UNIT:![0-9]+]] = distinct !DICompileUnit
-; CHECK-SAME: file: [[FILE:![0-9]+]],
-; CHECK-SAME: producer: "machine-outliner",
-; CHECK-SAME: isOptimized: true, runtimeVersion: 0, emissionKind: NoDebug
-
 ; CHECK: distinct !DISubprogram(name: "OUTLINED_FUNCTION_1",
-; CHECK-SAME: scope: [[FILE]],
-; CHECK-SAME: file: [[FILE]],
+; CHECK-SAME: scope: !1,
+; CHECK-SAME: file: !1,
 ; CHECK-SAME: type: [[TYPE:![0-9]+]],
 ; CHECK-SAME: isLocal: false, 
 ; CHECK-SAME: isDefinition: true,
 ; CHECK-SAME: flags: DIFlagArtificial,
 ; CHECK-SAME: isOptimized: true,
-; CHECK-SAME: unit: [[UNIT]],
+; CHECK-SAME: unit: !0,
 ; CHECK-SAME: variables: [[VARS:![0-9]+]]
 
-; CHECK: distinct !DISubprogram(name: "OUTLINED_FUNCTION_1",
-; CHECK-SAME: scope: [[FILE]],
-; CHECK-SAME: file: [[FILE]],
+; CHECK: distinct !DISubprogram(name: "OUTLINED_FUNCTION_0",
+; CHECK-SAME: scope: !1,
+; CHECK-SAME: file: !1,
 ; CHECK-SAME: type: [[TYPE]],
 ; CHECK-SAME: isLocal: false, 
 ; CHECK-SAME: isDefinition: true,
 ; CHECK-SAME: flags: DIFlagArtificial,
 ; CHECK-SAME: isOptimized: true,
-; CHECK-SAME: unit: [[UNIT]],
+; CHECK-SAME: unit: !0,
 ; CHECK-SAME: variables: [[VARS]]
 
 attributes #0 = { noinline noredzone nounwind optnone ssp uwtable "no-frame-pointer-elim"="true"  }
 attributes #1 = { nounwind readnone speculatable }
 attributes #2 = { noredzone }
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4, !5, !6}