[PATCH] D45593: [DebugInfo][OPT] Fixing a couple of DI duplication bugs of CloneModule

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Basically looks good, a few more comments would be nice (see inline).



================
Comment at: lib/IR/DebugInfo.cpp:179
   processScope(SP->getScope().resolve());
+  processCompileUnit(SP->getUnit());
   processType(SP->getType());
----------------
rtereshin wrote:
> Fixing case 2 (see the summary).
Let's just add a comment into the code here explaining why this call is necessary.


================
Comment at: lib/IR/DebugInfo.cpp:111
 
+void DebugInfoFinder::processCompileUnit(DICompileUnit *CU) {
+  if (!addCompileUnit(CU))
----------------
Can you add a comment explaining why this is necessary (like what you wrote for the phabricator review)?


================
Comment at: lib/IR/DebugInfo.cpp:137
+    else if (auto *M = dyn_cast<DIModule>(Entity))
+      processScope(M->getScope());
+  }
----------------
else llvm_unreachable("unexpected imported entity type")?


================
Comment at: test/DebugInfo/X86/clone-module-2.ll:10
+; compiled with `clang -O1 -g1 -emit-llvm -S`
+
+; CHECK:     DICompileUnit
----------------
Can you add a comment explaining what behavior is being tested here?
I.e.: why would running opt invoke the cloner? Is there perhaps a more targeted pass that we could run instead?


================
Comment at: tools/opt/opt.cpp:767
+    // If requested, run all passes twice with the same pass manager to catch
+    // bugs caused by persistent state in the passes.
     std::unique_ptr<Module> M2(CloneModule(*M));
----------------
Ah I see.


Repository:
  rL LLVM

https://reviews.llvm.org/D45593