[PATCH] D78851: Debug Info Support for Basic Block Sections

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2122-2123
+    if (&MBB == &MF->front() || MBB.isBeginSection())
+      TheCU.addRange({Asm->MBBSectionRanges[MBB.getSectionID()].BeginLabel,
+                      Asm->MBBSectionRanges[MBB.getSectionID()].EndLabel});
+    if (!MF->hasBBSections())
----------------
tmsriram wrote:
> dblaikie wrote:
> > Could we iterate MBBSectionRanges (it looks like it's built per-function?) instead of iterating BBs and looking up MBBSectionRanges? Looks like /maybe/ this code could be:
> > 
> > ```
> > for (const auto &R : Asm->MBBSectionRanges)
> >   TheCU.addRange(R.second.BeginLabel, R.second.EndLabel);
> > ```
> > 
> > (similarly for the code in updateSubprogramScopeDIE)
> IndexedMap does not support begin() end() iterators, so this won't work right?  This is a good idea but I might have to find a different Map data structure that is viable.  A DenseMap variant looks good as section IDs can be mapped to a small dense set of integers,  but DenseMap does not support this either?
Ah, I see - yeah, I think it'd probably be fine to just use a `DenseMap<MCSection*, LabelRangeThing>`, but I can see the nice-ness of the IndexedMap.

Are the section IDs unique just to the BB sections of one function? Otherwise it seems inefficient to have a map that's as big as all the sections in the output object file to be able to do indexed lookup into, compared to a denser map data structure - but perhaps it's still a small enough number that flat/indexed lookup is better.

In any case, it probably wouldn't be too hard to add begin/end filter_iterators to IndexedMap.

DenseMap should support iteration (implemented in DenseMapBase) - but has the problem of unreliable ordering (even for non-pointer keys, like ints - we shouldn't depend on the ordering of a hash data structure), so that'd make the output unstable, which isn't any good.

So maybe adding filtering iteration for IndexedMap is the right way to go - though I still wouldn't'd mind knowing just how big this IndexedMap gets - hmmm, I guess from the way it's built "getNumBlockIDs" means it is function-locally-unique, not whole-output-file-unique, which means it shouldn't be just as big as it needs to be. Sounds good.


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

https://reviews.llvm.org/D78851