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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:803-819
+  // Find the previous DIE whose parent is the same as the Die's parent.
   for (size_t I = getDIEIndex(Die); I > 0;) {
     --I;
-    if (DieArray[I].getDepth() == Depth - 1)
-      return DWARFDie();
-    if (DieArray[I].getDepth() == Depth)
+
+    Optional<uint32_t> PrevDieParentIdx = DieArray[I].getParentIdx();
+    assert((!PrevDieParentIdx || *PrevDieParentIdx < DieArray.size()) &&
+           "PrevDieParentIdx is out of DieArray boundaries");
----------------
I think this algorithm might be a bit slower than it needs to be. Maybe a bit harder to follow than it needs to be too?

Rather than checking every node from the current index back to the parent index - each node you visit that isn't a sibling, you can ask for its parent that'll let you jump potentially much further back in the index/closer to the parent.

eg: in the worst case, your previous sibling has a long chain of single children - then the algorithm I'm describing will be just as bad as a linear walk. But in the best case, where your previous sibling's first child has no children - no matter how many previous siblings that child has you can do this in only a couple of steps.

So I think the algorithm would look something like this:

```
// hmm, I might pull out the root die case differently:
size_t I = getDIEIndex(Die);
Optional<uint32_t> ParentIdx = Die->getParentIdx();
if (!ParentIdx) // root DIE
  return DWARFDie();
while (I > *ParentIdx)
  Optional<uint32_t> PrevDieParentIdx = DieArray[I].getParentIdx();
  // since we haven't reached the parent, this must have a valid parent (it's a sibling or a sibling's child)
  if (*PrevDieParentIdx == *ParentIdx)
    return DWARFDie(this, &DieArray[I]);
  // since we haven't reached the parent (the loop condition didn't break)
  // and we haven't reached a sibling (since we have inequal parent index) 
  // then we must be at a sibling's children - so try that DIE's parent next
  I = *PrevDieParentIdx;
}
```

Does that work/make sense?

Hmm, maybe it could be simplified further - there's no need to check the index every iteration because we can't skip the parent if the previous node isn't already our parent (or we're the root)... let's see:

```
if (!Die)
  return DWARFDie();
Optional<uint32_t> ParentIdx = Die->getParentIdx();
if (!ParentIdx) // root
  return DWARFDie();
size_t I = getDIEIndex(Die) - 1;
while (I != *ParentIdx)
  I = *DieArray[I].getParentIdx();
return DWARFDie(this, &DieArray[I]);
```


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:852
+  if (getDIEIndex(Die) == 0 && DieArray.size() > 1 &&
+      DieArray.back().getTag() == dwarf::DW_TAG_null) {
+    // For the unit die we might take last item from DieArray.
----------------
Why is this case checked for (rather than asserted) here - but for the non-root-DIE case above, it's asserted? Is this (as the TODO suggests) an invalidity that passes for the root DIE, but is caught earlier for non-root DIEs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110363