[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:22 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
> Sorry for the delay, I missed this somehow :/.  I'd prefer adding non-byte
> aligned reads/writes to lib/Support and adding unit tests for it there.
> More likely than not someone will need to do something similar at some
> point.
>
>> On 2015-Sep-14, at 06:13, Teresa Johnson <tejohnson at google.com> wrote:
>>
>> Ping - Duncan, quick question below regarding one of your suggestions
>> before I continue the commits from this patch. I am proposing keeping
>> the now-simplified non-byte aligned backpatching support in the
>> bitstream writer rather than adding that to lib/Support.
>>
>> On Thu, Sep 10, 2015 at 7:34 AM, 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.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> +    } 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);
>>>   }
>>> }
>>>
>>>
>>> --
>>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>



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