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

[Derek Schuff]

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?)
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.

>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?

thanks,
-Derek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111128/82035ef2/attachment.html>