[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: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");
----------------
avl wrote:
> dblaikie wrote:
> > 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]);
> > ```
> Yeah. Good idea. Will do.
I implemented variant looking more like first version. Because we need to have additional variable keeping prev die index:

while (I != *ParentIdx)    <<< if I equals to ParentIdx 
  I = *DieArray[I].getParentIdx();
return DWARFDie(this, &DieArray[I]); <<< then we return not a prev sibling but parent.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110363