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

[Derek Schuff]

Thanks for the review, replies inline.

On Wed, Jan 11, 2012 at 9:04 PM, Chris Lattner <clattner at apple.com> wrote:

>
> On Jan 9, 2012, at 2:12 PM, Derek Schuff wrote:
>
> > Happy new year, and happy 2-month anniversary to this thread. Here it is
> again, rebased to r147787:
> >
>
> Some comments on the patch:
>
> --- a/tools/llc/llc.cpp
> +++ b/tools/llc/llc.cpp
>
> +static cl::opt<bool>
> +LazyBitcode("streaming-bitcode",
> +  cl::desc("Use lazy bitcode streaming for file inputs"),
> +  cl::init(false));
>
> Instead of making lazy bitstreaming an optional feature, you should just
> switch something to use it by default, otherwise it won't get tested.
> llvm-dis would be a reasonable one, because we really don't care about a
> performance hit there.
>

Sounds reasonable.


>
>  // Build up all of the passes that we want to do to the module.
> -  PassManager PM;
> +  OwningPtr<PassManagerBase> PM;
> +  if (LazyBitcode)
> +    PM.reset(new FunctionPassManager(&mod));
> +  else
> +    PM.reset(new PassManager());
>
> Why is this needed?
>
>
FunctionPassManager runs only function passes, whereas PassManager runs
those plus module passes. Obviously, the whole module will not necessarily
be available when streaming.


>
> --- a/include/llvm/Bitcode/BitstreamReader.h
> +++ b/include/llvm/Bitcode/BitstreamReader.h
>
> +
> +class BitstreamBytes {
> +public:
> +  BitstreamBytes() { }
>
>
> How is this different than the code in llvm/Support/MemoryObject.h?  It
> seems that any additional functionality should be merged into memory object
> instead of making some new bitcode-specific streaming api.
>

Hm... not something I'm familiar with, I'll look into it.


> New classes should have doxygen comments on them.  If MemoryBitstreamBytes
> needs to continue to exist, it should take a StringRef instead of start/end
> char positions.  In any case, the abstraction for streaming has nothing to
> do with bitcode, so it should be in MemoryObject or somewhere else that is
> not the bitcode reader.
>
> +  // Returns the in memory address of Pos from the beginning of the
> stream.
> +  // May block until Pos bytes have been read, or EOF is reached.
> +  // Note that the first byte past the end may be skipped to, but may not
> have
> +  // its address taken.
> +  virtual const unsigned char *addressOf(size_t Pos) = 0;
>
> This seems like the wrong level of API for this.  AFAICT, this is all to
> support BLOBs in bitcode files.  You should just add an API to ensure that
> N bytes from the stream are available in memory somewhere.
>

Much of the convoluted-ness of the patch as it exists now is to try to
avoid having more than one copy of the bitcode in memory, and to avoid
unnecessary copies. But yes, this exists to support those BLOBs. I guess
they are uncommon enough that it might be worth potentially making an extra
copy to simplify the interface?


>
>
> This sort of change:
>
>           for (; NumElts; ++NextChar, --NumElts)
> -            Vals.push_back(*NextChar);
> +            Vals.push_back(BitStream->getBytes().getByte(NextChar));
>
> Makes me think that you don't have the right abstraction here.  Why is
> NextChar not some new kind of iterator that handles this?
>

I'll look into that too, along with MemoryObject.

>
>
> With this sort of change:
>     // If we run out of data, stop at the end of the stream.
> -    if (NextChar == BitStream->getLastChar()) {
> +    if (BitStream->getBytes().isEndPos(NextChar)) {
>
> you're definitely doing a good job raising the abstraction level of the
> bitcode reader, which is great.  However, why not (as a patch before the
> big reworking) add a "isEndPos()" method to the bitcode reader itself, and
> refactor code to use it?  Then in your big rework patch you only have to
> change the one method to indirect through bitstream, instead of changing
> every place that checks for the end of stream.
>
> --- /dev/null
> +++ b/include/llvm/Support/BitcodeStream.h
>
> This file doesn't seem like it needs to exist, the BitcodeStreamer
> shouldn't be exported at all, so it is just one C-style entrypoint, which
> can be added to some pre-existing header.
>
> +++ b/lib/Support/BitcodeStream.cpp
> @@ -0,0 +1,97 @@
>
> +namespace {
> +
> +const static error_code success;
>
> This is odd, why the global variable?
>
> IIRC this sort of mirrors some of the file-opening behavior in
MemoryBufffer.cpp where it has a similar global "success" error_code.


>
> Overall, my recommendation would be to split this into three patches: the
> first patch would just increase the abstraction level of the bitcode
> reader, by adding various predicates that you need and tidy things up.  The
> second would extend MemoryObject as needed to add the functionality that
> you need.  The third would actually switch the meat of the bitcode reader
> to be lazy-streaming, and switch a tool to use it.
>
>
OK; I'll look into that, thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120112/8b807960/attachment.html>