[PATCH] D110363: [DWARF][NFC] add ParentIdx and SiblingIdx to DWARFDebugInfoEntry for faster navigation.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:786-788
   // NULL DIEs don't have siblings.
   if (Die->getAbbreviationDeclarationPtr() == nullptr)
     return DWARFDie();
----------------
clayborg wrote:
> We don't need this if statement anymore as the sibling index won't be set for NULL DIEs.
Ok.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:799
 
 DWARFDie DWARFUnit::getPreviousSibling(const DWARFDebugInfoEntry *Die) {
   if (!Die)
----------------
clayborg wrote:
> clayborg wrote:
> > This function could be done down in DWARFDebugInfoEntry now.
> This is a complex function. Do we even use it? I would doubt that anyone ever asks for the previous DIE or uses a reverse iterator. It would be nice to remove this and the operator-- from the iterator, and the reverse_iterator for the DWARFDie and see if anyone is actually using it besides the unit test that tests it. IF we don't use it we can get rid of this and stop having to modify it if we update how DWARFDebugInfoEntry stores its data.
It is used in DWARFLinker.cpp:

```
for (auto Child : reverse(Die.children())) {
```

Probably that code might be refactored and usage of reverse iterator would not be necessary. I suggest to leave this refactoring for separate patch.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:859-871
+  uint32_t ChildIdx = DieIdx + 1;
+  while (ChildIdx < DieArray.size()) {
+    assert(*DieArray[ChildIdx].getParentIdx() == DieIdx && "Bad parent");
+
+    if (DieArray[ChildIdx].getTag() == dwarf::DW_TAG_null)
+      return DWARFDie(this, &DieArray[ChildIdx]);
+
----------------
clayborg wrote:
> What is this last while loop for? We should only need the normal case where SiblingIdx is set, or we have the unit die right?  Everything should have a sibling except the unit DIE. 
I assumed it to be used for incorrect dwarf case. But you are right - that loop might be safely removed even for incorrect dwarf case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110363/new/

https://reviews.llvm.org/D110363