<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 24, 2014 at 8:49 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">> That is to remove some calls to getSize()? Is there any expectation that<br>
> getPointer() will always a pointer within a contiguous region of memory?<br>
> That is, getPointer() is non-virtual and always refers to the Data field,<br>
> but the overriding implementation could still dynamically grow some buffer<br>
> and change Data to point to a new buffer (could that be "protected"<br>
> instead?)?<br>
<br>
</span>I think it would fail. Clang does things like<br>
<br>
* readRecord (... &Blob);<br>
* Save a pointer to Blob.<br>
* readRecord (...&Blob)<br>
<br>
It should be possible to use a list of buffers and add a<br>
getPointerImpl virtual if absolutely necessary, but see bellow.<br></blockquote><div><br></div><div>Ah okay, I forgot to look at what Clang does.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> *) It looks like gzip encoded files will have content-length set to the size<br>
> of the *gzipped* body, not the size of the original bitcode:<br>
> <a href="http://stackoverflow.com/questions/3819280/content-length-when-using-http-compression" target="_blank">http://stackoverflow.com/questions/3819280/content-length-when-using-http-compression</a>.<br>
> So we'll need to be careful and treat that as "unknown", which is a bit<br>
> unfortunate because gzip helps reduce bandwidth requirements.<br>
<br>
</span>You should try removing the need for getSize, but if that fails, this<br>
is so pnacl specific, can't you wrap the bitcode in with a header<br>
including the size (this would be kept in the PNaCl tree. It would<br>
just be another implementation of the DataStream).<br></blockquote><div><br></div><div><br></div><div>Sure, a wrapper is another option. Will have to discuss that with Derek and the other PNaCl folks when they get back, though it might just be that taking streaming out of upstream llvm and keeping it in our own reader is fine.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> Not quite clear to me that this is the same. Do we have a test of llvm-dis<br>
> w/ files that have a bitcode wrapper + misc data surrounding the true<br>
> bitcode contents?<br>
<br>
</span>We have tests that have a wrapper.<br>
<span class=""><br>
> The old dropLeadingBytes would track how many bytes were skipped so that<br>
> later, reading address N would mean reading N + BytesSkipped. It also<br>
> subtracted the skipped bytes from BytesRead, and I haven't looked closely<br>
> enough to see if there is similar state being tracked anymore.<br>
<br>
</span>There isn't. The point is precisely that it is not needed.<br></blockquote><div><br></div><div>Okay great, that's certainly much simpler then =)</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> @@ -218,18 +214,13 @@ public:<br>
>    void freeState();<br>
><br>
>    bool canSkipToPos(size_t pos) const {<br>
> -    // pos can be skipped to if it is a valid address or one byte past the<br>
> end.<br>
> -    return pos == 0 || BitStream->getBitcodeBytes().isValidAddress(<br>
> -        static_cast<uint64_t>(pos - 1));<br>
> +    size_t Size = BitStream->getSize();<br>
> +    return pos < Size;<br>
>    }<br>
><br>
> Is the "pos can be skipped ... or one byte past the end" behavior still<br>
> needed, or was it some artifact of how isValidAddress worked?<br>
<br>
</span>Doesn't show up on check-all or a lto bootstrap.<br>
<br>
<br>
I have looked at this a bit more, and now I have a more serious issue<br>
with the current state of trunk. Check the small attached patch. It<br>
just changes llc to use lazy reading of function bodies (not even lazy<br>
streaming). With that patch, llc fails with verifier check failures or<br>
assertions if the verifier is disabled.<br>
<br>
So, codegen cannot handle not reading all bodies upfront. How can<br>
PNaCl be getting any improvements from this? I understand that codegen<br>
of a function at a time is something you might want to add a some<br>
point, but lazy streaming was added in Feb 6 22:30:29 2012 and we<br>
still don't have it.<br></blockquote><div><br></div><div><div>Here's what I think happens (the PNaCl tree has some additional changes to make it work).</div><div><br></div><div>When you switch to getLazyIRFileModule, file reading is suspended once a function body block is encountered, so the function bodies are not materialized yet. Something needs to materialize them. Otherwise, were you seeing that the verifier couldn't find an entry block?</div><div><br></div><div>See the attached patch, which modifies the FPPassManager::runOnModule to materialize functions before running passes on them, as FunctionPassManager::run(Function &F) would have done. For PNaCl we had been using a FunctionPassManager directly, instead of a PassManager/FPPassManager to get a similar effect (see commented out code in the attached patch).</div><div><br></div><div>In general PNaCl's version of llc has been careful about which ModulePasses run during CodeGen. If ModulePasses touch the function bodies, then it could materialize the function bodies too early for streaming to be useful. So far, we only have a limited set of Module passes that we run outside of a PassManager. We also don't allow blockaddresses in our subset of the IR, which would have also caused some forward-referenced functions to be materialized.</div></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
It seems the best thing to do is delete this. It can always come back<br>
if it has a clean implementation and is actually useful (even if only<br>
at -O0).<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>