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

[Jonathan Roelofs via llvm-commits]

jroelofs added inline comments.

================
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");
----------------
loladiro wrote:
> 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.
How do I follow the indirection in that case?

================
Comment at: tools/llvm-project/llvm/lib/Transforms/IPO/GlobalOpt.cpp:1806
@@ +1805,3 @@
+                                   Scope);
+        DIB->insertDeclare(Alloca, Variable, E, DL, Store);
+      }
----------------
loladiro wrote:
> 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.
Is that the only thing it needs the store for? If so, maybe I can finagle another API to make it insertAfter the alloca when the Store arg is null.

================
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;
----------------
loladiro wrote:
> 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.
ok


http://reviews.llvm.org/D16209