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

[Teresa Johnson via llvm-commits]

On Tue, Sep 15, 2015 at 6:36 PM, Teresa Johnson <tejohnson at google.com> wrote:
> +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?

Not a good way to unit test this. Until I commit the rest of this
patch the only caller of BackpatchWord is 32-bit aligned. And I don't
think it is possible even once the VST change goes in to add a test
that forces the VST offset to have a specific alignment (I could
concoct one that is 8-bit but not 32-bit aligned today, but with
future bitcode changes that will drift). However, I do know that there
are quite a lot of tests that will now hit this fast path due to being
8-bit but not 32-bit aligned with the rest of this patch that adds the
call for the VST offset (confirmed by forcing it to fail and running
the tests).

>>
>>>>>>> +    } 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.

Ok, sure. I think I will leave it here for now so that I can get the
rest of this patch in and queue up the follow-on patch (adding the
function summaries etc) for review, and then split it out and figure
out how to add the unit tests, etc while the follow-on is out for
review.

Thanks,
Teresa

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



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