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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin marked 8 inline comments as done.
rtereshin added inline comments.


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


================
Comment at: lib/IR/DebugInfo.cpp:111
 
+void DebugInfoFinder::processCompileUnit(DICompileUnit *CU) {
+  if (!addCompileUnit(CU))
----------------
rtereshin wrote:
> aprantl wrote:
> > Can you add a comment explaining why this is necessary (like what you wrote for the phabricator review)?
> Sure, will do.
I'm adding the comment to `processSubprogram`s implementation at `processCompileUnit`'s call-site, as the `processCompileUnit` being a separate method is just an obvious decomposition / DRY thing.


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


================
Comment at: test/DebugInfo/X86/clone-module-2.ll:10
+; compiled with `clang -O1 -g1 -emit-llvm -S`
+
+; CHECK:     DICompileUnit
----------------
aprantl wrote:
> 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?
Done, see below.


================
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));
----------------
rtereshin wrote:
> 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.
I've added the comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D45593