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

Derek Schuff dschuff at google.com
Tue Dec 6 13:08:54 PST 2011


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/20111206/07d8aa5c/attachment.html>


More information about the llvm-commits mailing list