[PATCH] D16209: Teach GlobalOpt not to drop DebugInfo on the floor when it promotes globals to allocas

[Keno Fischer via llvm-commits]

loladiro added a subscriber: loladiro.

================
Comment at: tools/llvm-project/llvm/lib/Transforms/IPO/GlobalOpt.cpp:1796
@@ +1795,3 @@
+    if (DIGlobalVariable *D = FindDebugInfo(GV)) {
+      if (DIType *T = dyn_cast<DIType>(D->getRawType())) {
+        DEBUG(dbgs() << "                   ... and transferring its DebugInfo\n");
----------------
jroelofs wrote:
> This ^ line in particular feels weird to me. I thought I should use D->getType().resolve(), but couldn't figure out how to get ahold of the parameter that it wants.
Hmm, yeah, that's awkward. I don't think there's a good reason why the DIBuilder can't also take a type ref. In any case, this doesn't work because it could be an indirect reference.

================
Comment at: tools/llvm-project/llvm/lib/Transforms/IPO/GlobalOpt.cpp:1798
@@ +1797,3 @@
+        DEBUG(dbgs() << "                   ... and transferring its DebugInfo\n");
+        auto *Scope = D->getScope();
+        auto *Variable = DIB->createAutoVariable(Scope,
----------------
jroelofs wrote:
> Apparently this ^ isn't the scope that DILocation wants... not sure how to get the one it's looking for.
`F->getSubprogram()`, where `F` is the function you're inserting into probably makes the most sense.

================
Comment at: tools/llvm-project/llvm/lib/Transforms/IPO/GlobalOpt.cpp:1806
@@ +1805,3 @@
+                                   Scope);
+        DIB->insertDeclare(Alloca, Variable, E, DL, Store);
+      }
----------------
jroelofs wrote:
> Do I also need to insert the value one too?
No, but this will segfault if Store (the argument indicates which instruction to insert before) is null. I suppose ideally this would be the instruction after either the store (if not null) or the alloca (if null). I wonder if DIBuilder should grow some new APIs to just create the instruction, and then insert it with an IRBuilder.

================
Comment at: tools/llvm-project/llvm/lib/Transforms/IPO/GlobalOpt.cpp:3092
@@ -3064,2 +3091,3 @@
 bool GlobalOpt::runOnModule(Module &M) {
+  DIB = llvm::make_unique<DIBuilder>(M, /*AllowUnresolved=*/ false);
   bool Changed = false;
----------------
jroelofs wrote:
> The `false` here is copy-pasta. I don't actually know if that's the value I want.
It probably doesn't matter, but `false` is the conservative choice here.


http://reviews.llvm.org/D16209