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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram marked an inline comment as done.
tmsriram 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())
----------------
dblaikie wrote:
> 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.
Yes, the section IDs correspond to the BB section of one function.  Worst case, each BB goes into one section and gets a entry in the map.  I do not have a average case number with section clusters and inter-procedural layout,  but is probably like one-third the number of basic blocks if I have to hazard a guess.

I can look at adding begin/end iterators to Indexed Map.  It looks like it is using a vector underneath which is pre-allocated in size.  If I have to scan the vector fully then it is probably not more efficient than what we have right now as it is still linear in the number of basic blocks and not sections, but I can take a closer look. 


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

https://reviews.llvm.org/D78851