[PATCH] Bitcode: Collect all MDString records into a single blob

[Mehdi Amini via llvm-commits]

+        // FIXME: Avoid this upgrade logic; it's costly in the common case, and
+        // incorrectly modifies all MDStrings.
+        String = Current.str();
+        llvm::UpgradeMDStringConstant(String);

Since we can't have old bitcode with METADATA_BULK_STRING_SIZES, and the only way to produce these deprecated "llvm.vectorizer.*" metadata is to write textual IR (for which we don't provide any guarantee). 
So I'd advocate to drop it here (any lit test that is using the old syntax should be updated, bitcode files may be checked in with the old names for auto-upgrade test purpose, but that won't exercise this code path any way).

+static unsigned createMDStringDataAbbrev(BitstreamWriter &Stream) {

There is one call site, for such a simple function are the other abbrev creation also using a separate function for creation?

+        return error("Invalid record");

The string could be more explicit (all of them).

Overall it is easy for me to understand because I have all the context. Someone that has no idea about what's going on would have hard time (OK admittedly the full state of the bitcode reader/writer is kind of like that, but that's not an excuse), so I think this could benefit some better comments here and there.

-- 
Mehdi




> On Mar 24, 2016, at 4:21 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2016-Mar-24, at 16:05, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> Optimize output of MDStrings in bitcode.  This emits them in big blocks
>> (currently 1024) in a pair of records:
>> - BULK_STRING_SIZES: the sizes of the strings in the block, and
>> - BULK_STRING_DATA: a single blob, which is the concatenation of all the
>> strings.
>> 
>> Inspired by Mehdi's similar patch, http://reviews.llvm.org/D18342, this
>> should (a) slightly reduce bitcode size, since there is less record
>> overhead, and (b) greatly improve reading speed, since blobs are super
>> cheap to deserialize.
>> 
>> I needed to add support for blobs to streaming input to get the test
>> suite passing.
>> - StreamingMemoryObject::getPointer reads ahead and returns the address
>>  of the blob.
>> - To avoid a possible reallocation of StreamingMemoryObject::Bytes,
>>  BitstreamCursor::readRecord needs to move the call to JumpToEnd
>>  forward so that getPointer is the last bitstream operation.
> 
> That didn't compile, due to a last minute variable rename :/.
> 
> Try this one.
> 
> <0001-Bitcode-Collect-all-MDString-records-into-a-singl-v2.patch>