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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


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


================
Comment at: lib/IR/DebugInfo.cpp:137
+    else if (auto *M = dyn_cast<DIModule>(Entity))
+      processScope(M->getScope());
+  }
----------------
aprantl wrote:
> else llvm_unreachable("unexpected imported entity type")?
Good idea, I'll try that.


================
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));
----------------
aprantl wrote:
> Ah I see.
Yeah, I think the fact that `-run-twice` is invoking cloner is self-evident enough to not elaborate on that, we already have regression tests that rely on it. They aren't very honest, btw, as prior to this patch the `opt`'s output is the original module, not the clone, so their `FileCheck`'s are practically useless. Still works as opt compares the output of the first and second run, so if they differ, it will return with a non-zero return code and the test will still catch it, but anyway that kind of thing is just misleading. I think with these changes it's a tad better as `opt` will at least output the clone.

As for commenting what tests actually test I think it still needs to be done, thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D45593