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

[Mehdi Amini via llvm-commits]

Sent from my iPhone

> On Mar 24, 2016, at 7:27 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>>> On 2016-Mar-24, at 19:22, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>> 
>>> 
>>> On 2016-Mar-24, at 18:46, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>> 
>>> 
>>> 
>>>> On Mar 24, 2016, at 6:12 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>> 
>>>> 
>>>>> On 2016-Mar-24, at 16:46, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>>> 
>>>>> +        // 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).
>>>> 
>>>> Fixed in r264373.
>>> 
>>> Nice!
>>> 
>>> +        // Test for upgrading !llvm.loop.
>>> +        HasSeenOldLoopTags |= mayBeOldLoopAttachmentTag(Current);
>>> 
>>> I still think this is not needed for the new block (you won't have old bitcode that can have this block). That is, unless Clang/LLVM is current producing these deprecated loop annotation (I hope not).
>> 
>> I think it's a little too subtle to mix and match where the upgrade
>> code is put.  Do you think this will cause a slowdown in practice?
>> 
>>>>> +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?
>>>> 
>>>> Yes, the other metadata abbreviations are in functions.  I think it's
>>>> a cleaner model because both the `unsigned` and the `BitCodeAbbrev`
>>>> are rightly called some variation of "Abbrev".  With a separate
>>>> function you don't have to worry about which name refers to which
>>>> (since they are never in the same scope).
>>>> 
>>>>> +        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.
>>>> 
>>>> I was following the current error style.  You're right; no reason not
>>>> to set a new standard.
>>> 
>>> Somehow, I'd rather see a comment header for this function:
>>> 
>>> +static void writeMDStrings(ArrayRef<const Metadata *> Strings,
>>> 
>>> Something in the lines of:
>>> 
>>> /// Emit the MDStrings altogether. For efficiency we emit MDStrings using a 
>>> // pair of record, the first one is an array of integers indicating the size of 
>>> // each MDString encoded by the pair. The second record contains all the
>>> // MDStrings concatenated as a blob.
>>> static void writeMDStrings(ArrayRef<const Metadata *> Strings,
>>> 
>>> And in the reader:
>>> 
>>>  case bitc::METADATA_BULK_STRING_SIZES: {
>>>    /// See writeMDStrings for the description of MDString encoding.
>>> 
>>> 
>>> If the reader/writer were documented everywhere this way I'd have found it a lot more friendly when I had to jump in a few months ago.
>> 
>> Sure, I'll have a pass at updating the comments.
> 
> Also, please let me know if/when you're happy moving the remaining
> review to post-commit

Yes!

-- 
Mehdi

>> 
>>> 
>>> 
>>>> Please have another look.
>>> 
>>> 
>>> +  // Emit strings in large blocks to reduce record overhead.
>>> +  const size_t NumStringsPerBlob = 1024;
>>> 
>>> How is this constant chosen? This looks quite arbitrary, especially since it does not say anything about the size of the blob itself.
>> 
>> It's big enough to convince me that there isn't any overhead,
>> and small enough to avoid an unnecessarily large allocation
>> for the `Record` in the BitcodeReader.
>> 
>> Maybe 1024 is too big, even.  That'll be 8096 bytes.  In my
>> next patch I'll set it to 512 so that it's within a page on
>> most platforms.
>> 
>> Frankly, I'm happy with anything as big as 128; I'm not
>> interested in micro-optimizing this.
>> 
>>> Why not emitting everything in a single record? (I guess we have a limit, but it is not clear how you constant guarantee that we stay under the limit).
>> 
>> (as discussed above)
>> 
>>> 
>>> +  // Put the strings first.
>>> +  std::stable_partition(MDs.begin(), MDs.end(),
>>> +                        [](const Metadata *MD) { return isa<MDString>(MD); });
>>> 
>>> I assume the use of std::stable_partition instead of std::partition is on the same level as "-preserve-bc-uselistorder", i.e. paranoid reproducibility but not required for correctness?
>> 
>> Not paranoid at all.  This is reproducibility at different levels.
>> 
>> This makes it so that clang will produce the same output when run
>> twice on the same input.
>> 
>> Preserving use-list order let's you re-enter the compiler and get
>> the same result as if you hadn't left in the first place.
>> 
>>> const SmallVectorImpl<const LocalAsMetadata *> &getFunctionLocalMDs() const {
>>>   return FunctionLocalMDs;
>>> }
>>> +
>>> const TypeList &getTypes() const { return Types; }
>>> 
>>> New line is a spurious change?
>> 
>> Not spurious.  I'm adding more metadata-related functions to the
>> group; for clarity, I separated the metadata functions from the
>> non-metadata functions with a newline.
>> 
>>> 
>>> 
>>> -- 
>>> Mehdi
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>