[PATCH] D12536: Function bitcode index in Value Symbol Table and lazy reading support

Tue Sep 15 18:32:24 PDT 2015

+cc llvm-commits  (which somehow got dropped despite being a subscriber).
Teresa

On Wed, Sep 9, 2015 at 10:34 AM, Teresa Johnson <tejohnson at google.com> wrote:
> On Wed, Sep 9, 2015 at 10:15 AM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>>
>>> On 2015-Sep-09, at 08:06, Teresa Johnson <tejohnson at google.com> wrote:
>>>
>>> On Tue, Sep 8, 2015 at 5:41 PM, Duncan P. N. Exon Smith
>>> <dexonsmith at apple.com> wrote:
>>>>
>>>>> On 2015-Sep-08, at 16:26, Teresa Johnson <tejohnson at google.com> wrote:
>>>>>
>>>>> Duncan, thanks for the review, will address the other comments
>>>>> shortly. But one quick response on this issue:
>>>>>
>>>>> On Tue, Sep 8, 2015 at 12:59 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>>>>
>>>>>>> On Sep 8, 2015, at 11:40 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>>>>
>>>>>>>> +  // Emit the placeholder
>>>>>>>> +  SmallVector<unsigned, 2> Vals{llvm::bitc::MODULE_CODE_VSTOFFSET, 0};
>>>>>>>
>>>>>>> I don't remember seeing brace initializers much in our codebase; I can't
>>>>>>> remember whether that's a style thing, or an MSVC limitation, but I'm
>>>>>>> wary of them.
>>>>>>
>>>>>> That was my suggestion, I didn’t know about the issue with style or MSVC, sorry Teresa.
>>>>>>
>>>>>> The dev guide talks about that: http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor
>>>>>>
>>>>>> It seems the expected spelling in this case IIUC is:
>>>>>>
>>>>>> SmallVector<unsigned, 2> Vals = {llvm::bitc::MODULE_CODE_VSTOFFSET, 0};
>>>>>>
>>>>>> (added equals before the brace)
>>>>>>
>>>>>>>
>>>>>>> But in this case, it feels like you should be able to use:
>>>>>>>
>>>>>>>  unsigned Vals[] = {llvm::bitc::MODULE_CODE_VSTOFFSET, 0};
>>>>>>>
>>>>>>> except...
>>>>>>>
>>>>>>>> +  Stream.EmitRecordWithAbbrev(VSTOffsetAbbrev, Vals);
>>>>>>>
>>>>>>> ... this seems to take a SmallVectorImpl :(.  Not for a good reason,
>>>>>>> though, AFAICT.
>>>>>>>
>>>>>>> Can you try (as a separate NFC commit or series thereof) converting
>>>>>>> `EmitRecordWithAbbrevImpl()` and `EmitRecordWith*()` to take an
>>>>>>> `ArrayRef<uintty>` instead of `SmallVectorImpl<uintty>&`?
>>>>>>
>>>>>> I didn’t dare suggesting it, but it seems like the best solution if possible.
>>>>>
>>>>> Unfortunately this doesn't work. I get errors like:
>>>>>
>>>>> note:   ‘llvm::SmallVector<unsigned int, 64u>’ is not derived from
>>>>> ‘llvm::ArrayRef<T>’
>>>>>
>>>>> I'm only able to pass a SmallVector to an ArrayRef if the ArrayRef
>>>>> parameter is instantiated with the same type (e.g. I can pass a
>>>>> SmallVector<unsigned int, 64u> to an ArrayRef<unsigned int>).
>>>>
>>>> Weird.  It looks like the implicit constructor from
>>>> `const SmallVectorImpl<T>&` to `ArrayRef<T>` is broken.  I can't figure
>>>> out what's wrong with it.
>>>>
>>>> I thought I had things working somehow in r247107 by having `EmitRecord()`
>>>> explicitly wrap the vector with `makeArrayRef()` (which doesn't seem to
>>>> be broken...), but had to revert in r247108 because of clang users I
>>>> hadn't checked for.
>>>
>>> I played around a bit this morning and was also not able to figure out
>>> why the implicit constructor didn't work or how to get one to work.
>>>
>>> I have two solutions that both work:
>>> 1. Use your r247107 change and modify all of the clang callsites to
>>> use makeArrayRef.
>>> 2. Use your r247107 but add versions of EmitRecordWithAbbrev and
>>> EmitRecordWithBlob that take a SmallVectorImpl<T>& and invoke the
>>> corresponding ArrayRef<T> version using makeArrayRef. These are the
>>> interfaces used directly by clang.
>>>
>>> I prefer 2 since it is a lot less changes and looks cleaner. WDYT?
>>
>> I agree, (2) is a good incremental step.  You might const-ify the
>> `SmalLvectorImpl<T>&` parameters while you're at it.
>
> Mehdi indicated on IRC this morning that he is working on a variant of
> 1) (change clang, but use ArrayRef wherever possible).
>
>>
>>>
>>>>
>>>>> So perhaps for this patch I should go back to the way I had set this
>>>>> up in my earlier version, with the Vals setup using push_back, which
>>>>> is consistent with the rest of the file.
>>>>>
>>>>> Teresa
>>>>
>>>> Up to you.  Really the best option would be to fix `EmitRecord()` so
>>>> that it also takes an `ArrayRef<>` -- and stops doing the crazy linear
>>>> time push_front() -- and then you can just use `EmitRecord()` directly.
>>>> If you've run out of time to push on that, it's okay with me if you
>>>> just follow the file.
>>>
>>> I just want to decouple them so my other changes aren't held up while
>>> we figure out the right solution here.  For now I have changed my
>>> patch to use the " = {...}" initialization form suggested by Mehdi.
>>> But I will change it back if/when I can get an ArrayRef interface in.
>>
>> SGTM.
>>
>>> Note that EmitRecord can't easily just take an ArrayRef<T>. This is
>>> because it inserts the given Code at the start of the array. So that
>>> interface should probably stay as-is, otherwise it needs to go through
>>> a SmallVector or std::vector anyway to construct the new array.
>>
>> I agree it's not easy (and I agree that constructing a new array would
>> be the wrong solution).  I think `EmitRecordWithAbbrevImpl()` should be
>> refactored so that the code can be passed in separately from the rest
>> of the data.
>>
>> I think passing in `Optional<uintty> Code` might work, something like:
>> --
>> unsigned i = 0, e = static_cast<unsigned>(Abbv->getNumOperandInfos());
>> 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");
>>
>>    if (Op.isLiteral())
>>      EmitAbbreviatedLiteral(Op, Code.getValue());
>>    else
>>      EmitAbbreviatedField(Op, Code.getValue());
>> }
>>
>> for (; i != e; ++i) {
>>   // old code
>> }
>> --
>> This assumes that it's invalid to have a Code with an Array or Blob
>> abbreviation.  I suppose I'm not entirely sure?
>
> Yes, I think I agree that the Code should not be encoded in the abbrev
> as a blob or array.
>
> I can give this a shot, it seems independent from the ArrayRef change
> Mehdi is working on.
>
> Thanks,
> Teresa
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413


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