[PATCH] D12737: Pass an optional "code" to EmitRecordWithAbbrevImpl()

[Teresa Johnson via llvm-commits]

On Wed, Sep 9, 2015 at 2:22 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Sep-09, at 13:01, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>>
>> joker.eph created this revision.
>> joker.eph added a reviewer: dexonsmith.
>> joker.eph added a subscriber: llvm-commits.
>>
>> This avoids having EmitRecord() shifts the Vals array to insert the
>> code at the front.
>
> This LGTM with a few minor changes below.

I already committed a variant of this (r247186).

>
>>
>> http://reviews.llvm.org/D12737
>>
>> Files:
>>  include/llvm/Bitcode/BitstreamWriter.h
>>
>> Index: include/llvm/Bitcode/BitstreamWriter.h
>> ===================================================================
>> --- include/llvm/Bitcode/BitstreamWriter.h
>> +++ include/llvm/Bitcode/BitstreamWriter.h
>> @@ -287,9 +287,12 @@
>>   /// emission code.  If BlobData is non-null, then it specifies an array of
>>   /// data that should be emitted as part of the Blob or Array operand that is
>>   /// known to exist at the end of the record.
>> +  /// An optional Code can be passed separately as a convenience to avoid
>> +  /// having EmitRecord() performing a push_front on Vals.
>>   template <typename uintty>
>>   void EmitRecordWithAbbrevImpl(unsigned Abbrev, const ArrayRef<uintty> &Vals,
>
> We usually pass ArrayRefs by-value; no need for `const` and `&`.
>
>> -                                StringRef Blob) {
>> +                                StringRef Blob,
>> +                                Optional<uintty> Code = Optional<uintty>()) {
>
> This can be `Optional<uintty> Code = None`.

I can make that change to my commit, since I am passing in None for
the other callsites explictly right now.

>
>>     const char *BlobData = Blob.data();
>>     unsigned BlobLen = (unsigned) Blob.size();
>>     unsigned AbbrevNo = Abbrev-bitc::FIRST_APPLICATION_ABBREV;
>> @@ -299,8 +302,24 @@
>>     EmitCode(Abbrev);
>>
>>     unsigned RecordIdx = 0;
>> -    for (unsigned i = 0, e = static_cast<unsigned>(Abbv->getNumOperandInfos());
>> -         i != e; ++i) {
>> +    unsigned i = 0, e = static_cast<unsigned>(Abbv->getNumOperandInfos());
>> +
>> +    // If a "Code" is passed in separately, emit it first.
>> +    if (Code) {
>> +      assert(e && "Expected non-empty abbreviation");
>> +      const BitCodeAbbrevOp &Op = Abbv->getOperandInfo(i++);
>> +      assert(!Op.getEncoding() == BitCodeAbbrevOp::Array &&
>> +             !Op.getEncoding() == BitCodeAbbrevOp::Blob &&
>> +             "Expected literal or scalar code");

This assert did not quite work and my commit fixes it (the "! ... =="
changed to "!=" and also I nested this within the else below since
getEncoding does not like to be called when isLiteral is true).

>> +
>> +      if (Op.isLiteral())
>> +        EmitAbbreviatedLiteral(Op, Code.getValue());
>> +      else
>> +        EmitAbbreviatedField(Op, Code.getValue());
>> +    }
>> +
>> +    // Main loop, emitting all value in Vals.
>> +    for (; i != e; ++i) {
>>       const BitCodeAbbrevOp &Op = Abbv->getOperandInfo(i);
>>       if (Op.isLiteral()) {
>>         assert(RecordIdx < Vals.size() && "Invalid abbrev/record");
>> @@ -396,10 +415,8 @@
>>       return;
>>     }
>>
>> -    // Insert the code into Vals to treat it uniformly.
>> -    Vals.insert(Vals.begin(), Code);
>> -
>> -    EmitRecordWithAbbrev(Abbrev, makeArrayRef(Vals));
>> +    EmitRecordWithAbbrevImpl(Abbrev, makeArrayRef(Vals), StringRef(),
>
> I like the paint colour `""` better than `StringRef()`.
>
>> +                             Optional<uintty>(Code));
>
> I think `Code` should implicitly convert to `Optional<uintty>`; no need
> to call the constructor explicitly.

My committed patch doesn't use the cast. Note I had to use
Optional<unsigned> instead of Optional<uintty> in
EmitRecordWithAbbrevImpl (regardless of whether a cast is used here,
it didn't compile correctly with uintty).

>
>>   }
>>
>>   /// EmitRecordWithAbbrev - Emit a record with the specified abbreviation.
>>
>>
>> <D12737.34361.patch>
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413