[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");
----------------
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.


================
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.
----------------
dblaikie wrote:
> 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?
yeas, the reason is an invalidity that passes for the root DIE, but is caught earlier for non-root DIE. The idea is that if SiblingIdx is set in DWARFUnit::extractDIEsToVector then format is good and assertion must be satisfied. But we cannot rely on SiblingIdx of the root DIE. That is why we have run-time check for the root die and assertion for others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110363