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

[Sriraman Tallam via Phabricator via llvm-commits]

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())
----------------
tmsriram wrote:
> 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. 
TLDR; MapVector seems to work great for this use case, lookups and iterations are fast. Deletions are slow but we don' do this.

+ IndexedMap is not what we want. Maybe I just don't get it but it is basically a vector underneath and the mapping of key->integers is left to the user.  If it is sparse, iterating becomes an issue as we have to go over non-existent elements.  It also requires early resizing.  My feeling is IndexedMap was never made to be iterated.
+ MapVector is still off DenseMap and this seems to work perfect here.  A plain vector would do if we solve the block order problem which you noted.  


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

https://reviews.llvm.org/D78851