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

[Derek Schuff]

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/20111201/02cc5064/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bitcode_streaming_r145582.diff
Type: text/x-patch
Size: 41672 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111201/02cc5064/attachment.bin>