[llvm-commits] Proposal/patch: Enable bitcode streaming

[Derek Schuff]

ping #2...

On Tue, Dec 6, 2011 at 1:08 PM, Derek Schuff <dschuff at google.com> wrote:

> Ping? I believe I've addressed all the comments.
> Also (assuming approval), someone will have to commit this, since I'm not
> yet a committer.
>
>
> On Thu, Dec 1, 2011 at 11:23 AM, Derek Schuff <dschuff at google.com> wrote:
>
>> Thanks for the review, comments have been addressed
>> (per IRC discussion: virtual destructors left in place because moving
>> them to BitcodeReader.cpp breaks linking clang, which includes
>> BitstreamReader.h for AST serialization but does not depend on bitcode lib)
>>
>> Patch attached.
>>
>>
>> On Wed, Nov 30, 2011 at 2:39 PM, Nick Lewycky <nlewycky at google.com>wrote:
>>
>>> On 28 November 2011 12:42, Derek Schuff <dschuff at google.com> wrote:
>>>
>>>> thanks for the feedback, I'm fixing all the suggestions, but I do have
>>>> one question:
>>>>
>>>> On Mon, Nov 21, 2011 at 3:38 PM, Nick Lewycky <nlewycky at google.com>wrote:
>>>>
>>>>> This is in a header file. Please move the implementation of the
>>>>> virtual destructor to BitcodeReader.cpp, or else copies will end up in
>>>>> every .o file that includes this header.
>>>>>
>>>>> Same with MemoryBitstreamBytes and LazyBitstreamBytes. More general
>>>>> question, any reason to put MemoryBitstreamBytes and LazyBitstreamBytes
>>>>> implementations inside the header? You could create
>>>>> lib/Bitcode/Reader/BitstreamReader.cpp.
>>>>>
>>>>>
>>>> The BitstreamBytes implementations are in the header file because
>>>> BitstreamReader/BitstreamCursor is implemented entirely in the header
>>>> file. Presumably BitstreamReader is in the header file so it can be
>>>> inlined. Presumably this improves performance (especially in llc where
>>>> there the usage is only in one file), although there seem to be several
>>>> uses of BitstreamCursor in clang, so it may contribute to more code size
>>>> bloat there. If you want I can move everything to BitstreamReader.cpp
>>>> (perhaps LTO could still buy us back any performance loss from inlining?)
>>>>
>>>
>>> That's a great point. Given the other code in the bitcode reader, I
>>> think it's fine to leave the implementations in the header -- except for
>>> the virtual destructors.
>>>
>>> It would also be possible to move just the BitstreamBytes
>>>> implementations into a cpp file using the common idiom of having static
>>>> factory functions with an anonymous namespace rather than just calling
>>>> "new" directly like we do right now.
>>>>
>>>
>>> I don't have a sense that either way would be particularly better.
>>>
>>>  >One other thing. The behaviour when LazyBitcode is true could be
>>>> really different from when it off on the same .bc file on >disk. It'd be
>>>> good to add comments pointing out why (module-level assembly comes to mind,
>>>> anything else?).
>>>>
>>>> Sure. where would be a good place to put it?
>>>>
>>>
>>> I was thinking of comments above the option in llc.
>>>
>>> Nick
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111209/f1699ca1/attachment.html>