[PATCH] Refactor debug info lexical block generation

[Amjad Aboud]

REPOSITORY
  rL LLVM

================
Comment at: include/llvm/CodeGen/DIE.h:191
@@ +190,3 @@
+  ///
+  void collapseChild(DIE* Child) {
+    assert(Child->getParent() == this);
----------------
friss wrote:
> This won't play nice with Duncan's rewrite of the DIE memory management that I think will land shortly.
I am not aware of this rewrite.
Can you point me to a reference where I can find out how it will look like?
Also, do you have other suggestions on how to do this?

================
Comment at: include/llvm/CodeGen/DIE.h:193-196
@@ +192,6 @@
+    assert(Child->getParent() == this);
+    for (auto &Grandchild : Child->Children) {
+      Grandchild->Parent = nullptr;
+      addChild(std::move(Grandchild));
+    }
+    Child->Parent = nullptr;
----------------
friss wrote:
> We are always adding the nodes at the end of the children list. Do we care about the order? Should we try to insert them in place of the collapsed node?
Agree, will fix that.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:535
@@ +534,3 @@
+    if (auto *Skeleton = TheU->getSkeleton())
+      Dies.push_back(&Skeleton->getUnitDie());
+  }
----------------
probinson wrote:
> I don't think skeleton DIEs can have lexical-block children?
> If not, then collecting them is unnecessary work, which means you don't need to collect the Units into their own DIE vector.
I am not much familiar with the Skeleton DIEs, but without collecting them, there was a test that failed, so I assume the Skeleton DIEs do have lexical blocks.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:545-549
@@ +544,7 @@
+    bool Skip = Die->getAbbrev().getTag() == dwarf::DW_TAG_lexical_block;
+    for (auto &Child : Die->getChildren()) {
+      Dies.push_back(&*Child);
+      Skip &= (Child->getAbbrev().getTag() == dwarf::DW_TAG_lexical_block ||
+               Child->getAbbrev().getTag() == dwarf::DW_TAG_inlined_subroutine);
+    }
+
----------------
friss wrote:
> This seems prohibitively expensive, you are adding each and every DIE in the tree to the worklist. I'm pretty sure you could filter to only take subprograms/blocks/inline_subroutines. (Even better, don't we already collect all the functions somewhere?)
> 
> This also seems wrong. You might remove inlined_subroutines with that change and we don't want that.
I will check if I can optimize this loop by traverse only over subprograms/blocks/inline_subroutines.

However, I do not have a bug here, I will never remove an inline_subroutine.
I am just collapsing lexical-blocks that does not have any child other than lexical blocks and/or inlined_subroutine.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:552
@@ +551,3 @@
+    if (Skip)
+      Die->getParent()->collapseChild(Die);
+  }
----------------
probinson wrote:
> If my understanding is correct, then you could set a breakpoint here and never hit it.
Not true, if you check line 546 I am pushing to the Dies vector the children of the compile unit, and their grandchildren and so on. At some point the Die will be a lexical block.
However, I will try improve this loop as Frederic suggested.

http://reviews.llvm.org/D9960

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/