[PATCH] D42254: Add optional DICompileUnit to DIBuilder and make outlined debug info use that CU

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:116
+  /// \p Candidate lives in, return it.
+  DICompileUnit *getCUIfExists() const {
+    assert(MF && "Candidate has no MF!");
----------------
How about `getCompileUnitOrNull`?


================
Comment at: lib/CodeGen/MachineOutliner.cpp:179
 
+  /// \brief If there is a DICompileUnit for any Candidate for this outlined
+  /// function, then return it. Otherwise, return nullptr.
----------------
Nitpick: there is no need to use `\brief` in Doxygen comments. LLVM uses the autobrief option.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:181
+  /// function, then return it. Otherwise, return nullptr.
+  DICompileUnit *getCUIfExists() const {
+    DICompileUnit *CU = nullptr;
----------------
perhaps shorter:
```
for (auto &C : Candidates) 
  if (auto *CU = C->getCUIfExists())
    return CU;
 return nullptr;
```


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1282
+
+    std::unique_ptr<DIBuilder> DB = llvm::make_unique<DIBuilder>(M, true, CU);
+    DIFile *Unit = DB->createFile(CU->getName(), CU->getDirectory());
----------------
Why not just allocate the DIBuilder on the stack?
`DIBuilder DB(M, true, CU)`


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1283
+    std::unique_ptr<DIBuilder> DB = llvm::make_unique<DIBuilder>(M, true, CU);
+    DIFile *Unit = DB->createFile(CU->getName(), CU->getDirectory());
+
----------------
Can you also grab the file from the DISubprogram that you got the CU from?


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1290
+          Unit /* Context */, F->getName(),
+          StringRef() /* Empty linkage name. */, Unit /* File */,
+          0 /* Line numbers don't matter*/,
----------------
I'd also put the linkage name here.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1291
+          StringRef() /* Empty linkage name. */, Unit /* File */,
+          0 /* Line numbers don't matter*/,
+          DB->createSubroutineType(DB->getOrCreateTypeArray(None)), /* void */
----------------
0 /* Line 0 is reserved for compiler-generated code */


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1292
+          0 /* Line numbers don't matter*/,
+          DB->createSubroutineType(DB->getOrCreateTypeArray(None)), /* void */
+          false, true, 0, /* Line in scope doesn't matter*/
----------------
rest looks good!


https://reviews.llvm.org/D42254