Thanks for the review, replies inline.<br><br><div class="gmail_quote">On Wed, Jan 11, 2012 at 9:04 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com">clattner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
On Jan 9, 2012, at 2:12 PM, Derek Schuff wrote:<br>
<br>
> Happy new year, and happy 2-month anniversary to this thread. Here it is again, rebased to r147787:<br>
><br>
<br>
</div>Some comments on the patch:<br>
<br>
--- a/tools/llc/llc.cpp<br>
+++ b/tools/llc/llc.cpp<br>
<br>
+static cl::opt<bool><br>
+LazyBitcode("streaming-bitcode",<br>
+ cl::desc("Use lazy bitcode streaming for file inputs"),<br>
+ cl::init(false));<br>
<br>
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.<br>
</blockquote><div><br></div><div>Sounds reasonable.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
// Build up all of the passes that we want to do to the module.<br>
- PassManager PM;<br>
+ OwningPtr<PassManagerBase> PM;<br>
+ if (LazyBitcode)<br>
+ PM.reset(new FunctionPassManager(&mod));<br>
+ else<br>
+ PM.reset(new PassManager());<br>
<br>
Why is this needed?<br>
<br></blockquote><div><br></div><div>FunctionPassManager runs only function passes, whereas PassManager runs those plus module passes. Obviously, the whole module will not necessarily be available when streaming.</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
--- a/include/llvm/Bitcode/BitstreamReader.h<br>
+++ b/include/llvm/Bitcode/BitstreamReader.h<br>
<div class="im"><br>
+<br>
+class BitstreamBytes {<br>
+public:<br>
+ BitstreamBytes() { }<br>
<br>
<br>
</div>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.<br></blockquote>
<div><br></div><div>Hm... not something I'm familiar with, I'll look into it. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br>
<br>
+ // Returns the in memory address of Pos from the beginning of the stream.<br>
+ // May block until Pos bytes have been read, or EOF is reached.<br>
+ // Note that the first byte past the end may be skipped to, but may not have<br>
+ // its address taken.<br>
+ virtual const unsigned char *addressOf(size_t Pos) = 0;<br>
<br>
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.<br></blockquote>
<div><br></div><div>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?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
This sort of change:<br>
<br>
for (; NumElts; ++NextChar, --NumElts)<br>
- Vals.push_back(*NextChar);<br>
+ Vals.push_back(BitStream->getBytes().getByte(NextChar));<br>
<br>
Makes me think that you don't have the right abstraction here. Why is NextChar not some new kind of iterator that handles this?<br></blockquote><div><br></div><div>I'll look into that too, along with MemoryObject. </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
With this sort of change:<br>
// If we run out of data, stop at the end of the stream.<br>
- if (NextChar == BitStream->getLastChar()) {<br>
+ if (BitStream->getBytes().isEndPos(NextChar)) {<br>
<br>
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.<br>
<br>
--- /dev/null<br>
+++ b/include/llvm/Support/BitcodeStream.h<br>
<br>
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.<br>
<br>
+++ b/lib/Support/BitcodeStream.cpp<br>
@@ -0,0 +1,97 @@<br>
<br>
+namespace {<br>
+<br>
+const static error_code success;<br>
<br>
This is odd, why the global variable?<br>
<br></blockquote><div>IIRC this sort of mirrors some of the file-opening behavior in MemoryBufffer.cpp where it has a similar global "success" error_code.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br>
<font color="#888888"><br></font></blockquote><div><br></div><div>OK; I'll look into that, thanks. </div></div>