[PATCH] Bitcode: Try to emit metadata in function blocks

[Mehdi Amini via llvm-commits]

> On Mar 28, 2016, at 7:44 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2016-Mar-27, at 17:41, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> Whenever metadata is only referenced by a single function, emit the
>> metadata just in that function block.  This should improve lazy-loading
>> by reducing the amount of metadata in the global block.
>> 
>> For now, this should catch all DILocations, and anything else that
>> happens to be referenced only by a single function.
>> 
>> It's also a first step toward a couple of possible future directions
>> (which this commit does *not* implement):
>> 
>> 1. Some debug info metadata is only referenced from compile units and
>>   individual functions.  If we can drop the link from the compile
>>   unit, this optimization will get more powerful.
>> 
>> 2. Any uniqued metadata that isn't referenced globally can in theory be
>>   emitted in every function block that references it (trading off
>>   bitcode size and full-parse time vs. lazy-load time).
>> 
>> Note: the only change on the reader side is cautious error checking.
>> The metadata stored in function blocks gets purged after parsing each
>> function, which means unresolved forward references will get lost.
>> Since all the global metadata should have already been resolved by the
>> time we get to the function metadata blocks we just need to check for
>> that case.  (If for some reason we need this, the fix is to store
>> about-to-be-dropped unresolved nodes in MetadataList::shrinkTo until
>> they can be handled succesfully by a future call to
>> MetadataList::tryToResolveCycles.)
>> 
>> <0001-Bitcode-Try-to-emit-metadata-in-function-blocks-v3.patch>
> 
> I realized that it's straightforward to use llvm-bcanalyzer to test that
> this works correctly.  Attaching a new version of the patch with a
> testcase.


I wanted to test it locally, but can't ThinLTO-link llvm-tblgen, it crashes with "error: Invalid record: metadata strings corrupt offset"

Some other comments (while I'm still trying to make sense of organizeMetadata()):

+  struct MDIndex {

one-line doxygen could be welcome

+  struct MDRange {

same

-  unsigned NumModuleMDs;
+  unsigned NumModuleMDStrings = 0;
+  unsigned NumBaseMDs = 0;
+  unsigned NumMDStrings = 0;
 
Same


  /// Purge function metadata.
  void purgeFunctionMetadata();

Doxygen that are 1-1 matching to the function name are useless, either there is something more useful to say, or it could be dropped if the name makes what it is intended for obvious.



+  // Check whether this is a new function.
+  if (Insertion.first->second.updateFunction(F))
+    if (auto *N = dyn_cast<MDNode>(MD))
+      NeedToDropFunctions.push_back(N);

The dyn_cast is not clear to me?


+    return std::make_tuple(LHS.F, !isa<MDString>(MDs[LHS.ID - 1]), LHS.ID) <
+          std::make_tuple(RHS.F, !isa<MDString>(MDs[RHS.ID - 1]), RHS.ID);

Things like the "- 1" offset that is leaking here seems like a missing abstraction. Not that it seems to be widespread, but could be hidden in a member of MDIndex.


+  // Return early if nothing happened.
+  if (!Order.back().F && !isa<MDString>(MDs[Order.front().ID - 1]))
+    return;

I'm glad there is a comment, without it I wouldn't know what's going on here!


-- 
Mehdi