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

[Teresa Johnson via llvm-commits]

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

On Tue, Sep 15, 2015 at 4:26 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
> (Working backwards...)
>
>> On 2015-Sep-10, at 07:34, Teresa Johnson <tejohnson at google.com> wrote:
>>
>> On Wed, Sep 9, 2015 at 12:39 PM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>>
>>>> On 2015-Sep-09, at 09:14, Teresa Johnson <tejohnson at google.com> wrote:
>>>>
>>>> On Tue, Sep 8, 2015 at 11:40 AM, Duncan P. N. Exon Smith
>>>> <dexonsmith at apple.com> wrote:
>>>>>
>>>>>> On 2015-Sep-08, at 06:43, Teresa Johnson <tejohnson at google.com> wrote:
>>
>>>>>
>>>>>> +    unsigned ByteNo = BitNo / 32 * 4;
>>>>>> +    if ((BitNo & 31) == 0) {
>>>>>> +      // Already 32-bit aligned
>>>>>> +      support::endian::write32le(&Out[ByteNo], NewWord);
>>>>>
>>>>> Does the fast path work for anything that's 8-bit aligned?  (I'm just
>>>>> guessing based on the change you made to the caller.)
>>>>
>>>> I assume you are asking whether the write32le works if it is 8-bit
>>>> aligned if we changed the guard here. Checking...
>>>
>>> Right.  Otherwise maybe we were just getting lucky before?
>>
>> No, before it was only called for 32-bit aligned addresses. This is
>> because the only caller was from ExitBlock to update the block size
>> field, which is always 32-bit aligned (in fact the BitNo at the
>> callsite is computed by multiplying the StartSizeWord by 32). This is
>> because the placeholder is emitted just after a FlushToWord() in
>> EnterSubblock.
>>
>> I did confirm that the fast path does also work when BitNo is byte
>> aligned, so it is simple to support that particular new case.
>
> Great.  Maybe add a unit test to confirm it?
>
>>>>>> +    } else {
>>>>>> +      // First patch in the lower 32-(BitNo&32) bits of NewWord, by
>>>>>> +      // shifting and masking into bits BitNo&32 to 32 of the first byte.
>>>>>> +      unsigned CurWord = support::endian::read32le(&Out[ByteNo]);
>>>>>> +      unsigned StartBit = BitNo & 31;
>>>>>> +      // Currently expect to backpatch 0-value placeholders.
>>>>>> +      assert((CurWord >> StartBit) == 0);
>>>>>> +      CurWord |= NewWord << StartBit;
>>>>>> +      support::endian::write32le(&Out[ByteNo], CurWord);
>>>>>> +
>>>>>> +      // Next patch in the upper BitNo&32 bits of NewWord, by
>>>>>> +      // shifting and masking into bits 0 to (BitNo&32)-1 of the second byte.
>>>>>> +      ByteNo += 4;
>>>>>> +      CurWord = support::endian::read32le(&Out[ByteNo]);
>>>>>> +      // Currently expect to backpatch 0-value placeholders.
>>>>>> +      assert((CurWord << (32 - StartBit)) == 0);
>>>>>> +      CurWord |= NewWord >> (32 - StartBit);
>>>>>> +      support::endian::write32le(&Out[ByteNo], CurWord);
>>>>>
>>>>> It's seems weird to have this logic here for unaligned writes.  Is there
>>>>> no functionality for this in llvm/Support?  If not, can you add it in a
>>>>> separate commit (with unit tests)?  (If so, just use it.)
>>
>>
>> In the new caller, the BitNo may not be even byte-aligned. In that
>> case some shifting and or-ing is required, although it turns out I can
>> do a single read and write via read64le and write64le to make it much
>> simpler. I don't see any support for doing something like this already
>> in lib/Support, do you still think it is useful to add this
>> functionality there? The new BackpatchWord is quite a bit simpler, the
>> following works:
>>
>>  /// Backpatch a 32-bit word in the output with the specified value.
>>  void BackpatchWord(uint64_t BitNo, unsigned NewWord) {
>>    unsigned ByteNo = BitNo / 8;
>>    if ((BitNo & 7) == 0) {
>>      // Already 8-bit aligned
>>      support::endian::write32le(&Out[ByteNo], NewWord);
>>    } else {
>>      uint64_t CurDWord = support::endian::read64le(&Out[ByteNo]);
>>      unsigned StartBit = BitNo & 7;
>>      // Currently expect to backpatch 0-value placeholders.
>>      assert(((CurDWord >> StartBit) & 0xffffffff) == 0);
>>      CurDWord |= NewWord << StartBit;
>>      support::endian::write64le(&Out[ByteNo], CurDWord);
>>   }
>> }
>
> This looks a lot better, but as I mentioned in my other mail, I think
> this would be useful to have in lib/Support (with a couple of unit
> tests).
>
> I'd be fine with you coming back and splitting it out as a follow-up
> though.



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