[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");
----------------
avl wrote:
> 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.
> 
> 
Oh, fair point about returning the parent - but I'd like to keep the code a bit simpler if possible.

Specifically handling the "I'm the root node" and "I have no previous sibling" up-front, and then letting the rest of the code only handle the "I'm eventually going to find my sibling" case - at least I /think/ that'd be more legible, but let's see..

```
  Optional<uint32_t> ParentIdx = Die->getParentIdx();
  if (!ParentIdx)
    return DWARFDie();
  uint32_t I = getDIEIndex(Die) - 1;
  if (I == *ParentIdx)  // immediately previous node is parent, there is no previous sibling
    return DWARFDie();
  while (DieArray[I].getParentIdx() != *ParentIdx)
    I = DieArray[I].getParentIdx();
  return DWARFDie(this, &DieArray[I]);
```

Maybe that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110363