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

[Mehdi Amini via llvm-commits]

OK, so I tested the patch but didn't see much runtime improvements.

On opt.cpp there are 120274 Metadata total, and 8789 will be localized (7679 DILocation total, all of them are localized).

It seems that we'll have to wait for Adrian's removal of the Subprograms field on the CU to start seeing real gains.

-- 
Mehdi

> On Mar 28, 2016, at 10:33 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2016-Mar-28, at 16:05, Mehdi Amini <mehdi.amini at apple.com> wrote:
>> 
>>> 
>>> On Mar 28, 2016, at 3:46 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>>> 
>>>> On 2016-Mar-28, at 15:42, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>> 
>>>> 
>>>>> 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"
>>> 
>>> Interesting.  If you've got a reproduction that would help.
>> 
>> $ ../install-clang/bin/clang -flto=thin -O2 -g -c lib/Support/regcomp.c -o - | ../install-clang/bin/opt -o /dev/null
>> <stdin>
>> error: Invalid record: metadata strings corrupt offset
>> 
>> Removing either -g or -O2 makes the problem go away.
>> 
>> -- 
>> Mehdi
>> 
> 
> Fixed in r264699 (this patch exposed a bug from r264551).
> 
>> 
>>> 
>>>> 
>>>> 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