[PATCH] D45593: [DebugInfo][OPT] Fixing a couple of DI duplication bugs of CloneModule
Adrian Prantl via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 13 10:04:48 PDT 2018
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
More information about the llvm-commits
mailing list