Duncan P. N. Exon Smith
2014-Dec-19 04:15 UTC
[LLVMdev] [Patches][RFC] What to do about bitcode streaming.
+llvmdev> On 2014 Dec 18, at 15:14, Rafael Espíndola <rafael.espindola at gmail.com> wrote: > > Currently we support reading bitcode in 3 ways: > > * Read everything upfront. > * Be lazy about reading the function bodies. > * Read the bitcode over a streaming interface. > > The first two modes are commonly used and well tested. In fact the > "read everything" mode is implemented by starting lazy and calling > materializeAllPermanently. > > The thirds mode is used in tree only by llvm-dis, which means that > some bugs can hide for quite some time (222505 for example). To the > best of my knowledge it is only real user is PNaCl. > > It also has a disproportional code complexity. There are two virtual > interfaces (DataStreamer and MemoryObject) and they are leaky: Not all > bitcode features are supported when streaming and there are a few "if > (LazyStreamer)" int the reader. > > I have attached two patches that solve the problem with different trade-offs. > > One just deletes the feature. This would make PNaCl responsible for > maintaining their own reader. I assume they will have to do it at some > point since they are looked to a fixed format, but this would make it > an immediate issue. > > The other one gets most of the simplifications by adding just one > assumption: the size is known. This should be true for http with > Content-Length. > > We go from 2 interfaces to 1 and that interface has a single virtual > method. There are no ifs on the data being streamed or missing > features. > > It might be even possible to drop the requirement for the size to be > known: Replace the call to AtEndOfStream by just trying to read more > and checking if it failed, but that is a bit more than I wanted to do > for this. > > Cheers, > RafaelI CC'ed llvmdev to put a few more eyes on the "what's the right direction?" question. IMO these both look like huge improvements. The streaming interface was the strangest part of the bitcode reader when I was trying to figure out how it all fit together for the use-list-order work. Personally I favour the "just delete it" path, but maybe there's something I'm missing, and the other patch looks great too. I didn't read carefully yet (although I noticed two quirks in the first patch that I've pointed out below) -- I'll have a closer look once you've decided on a direction.> diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp > index e228b1d..1322edd 100644 > --- a/lib/Bitcode/Reader/BitcodeReader.cpp > +++ b/lib/Bitcode/Reader/BitcodeReader.cpp > @@ -33,6 +32,26 @@ enum { > SWITCH_INST_MAGIC = 0x4B5 // May 2012 => 1205 => Hex > }; > > +BitcodeReader::BitcodeReader(MemoryBuffer * Buffer, LLVMContext & C)clang-format?> + : Context(C), TheModule(nullptr), Buffer(Buffer), > + SeenValueSymbolTable(false), ValueList(C), MDValueList(C), > + SeenFirstFunctionBody(false), UseRelativeIDs(false), > + WillMaterializeAllForwardRefs(false) { > diff --git a/tools/llvm-dis/llvm-dis.cpp b/tools/llvm-dis/llvm-dis.cpp > index 072f636..3e21164 100644 > --- a/tools/llvm-dis/llvm-dis.cpp > +++ b/tools/llvm-dis/llvm-dis.cpp > @@ -135,12 +205,13 @@ int main(int argc, char **argv) { > else > DisplayFilename = InputFilename; > ErrorOr<std::unique_ptr<Module>> MOrErr > - getStreamedBitcodeModule(DisplayFilename, Streamer, Context); > + getStreamedBitcodeModule(DisplayFilename, std::move(Streamer), Context); > if (std::error_code EC = MOrErr.getError()) > ErrorMessage = EC.message(); > else > M = std::move(*MOrErr); > if(M.get()) { > + errs() << "foobar " << Foo.getMaxPos() << '\n';Debug output?> if (std::error_code EC = M->materializeAllPermanently()) { > ErrorMessage = EC.message(); > M.reset();
Rafael Espíndola
2014-Dec-19 14:41 UTC
[LLVMdev] [Patches][RFC] What to do about bitcode streaming.
> I CC'ed llvmdev to put a few more eyes on the "what's the right > direction?" question. > > IMO these both look like huge improvements. The streaming interface was > the strangest part of the bitcode reader when I was trying to figure out > how it all fit together for the use-list-order work. > > Personally I favour the "just delete it" path, but maybe there's > something I'm missing, and the other patch looks great too. > > I didn't read carefully yet (although I noticed two quirks in the first > patch that I've pointed out below) -- I'll have a closer look once > you've decided on a direction.Thanks. Looks like the clang-format I had installed was a bit too old. I have also rebased and removed the debug output. Cheers, Rafael -------------- next part -------------- A non-text attachment was scrubbed... Name: remove-data-streamer.patch Type: text/x-patch Size: 39216 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141219/ed244e66/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: known-size.patch Type: text/x-patch Size: 40048 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141219/ed244e66/attachment-0001.bin>
JF Bastien
2014-Dec-19 20:59 UTC
[LLVMdev] [Patches][RFC] What to do about bitcode streaming.
Hi Rafael, Would you mind waiting for Derek to come back from vacation to discuss this? We do use this code and could improve how it's used and tested within LLVM. Derek is the best person to discuss this, he'll be back in mid-January. Thanks, JF On Fri, Dec 19, 2014 at 6:41 AM, Rafael Espíndola < rafael.espindola at gmail.com> wrote:> > > I CC'ed llvmdev to put a few more eyes on the "what's the right > > direction?" question. > > > > IMO these both look like huge improvements. The streaming interface was > > the strangest part of the bitcode reader when I was trying to figure out > > how it all fit together for the use-list-order work. > > > > Personally I favour the "just delete it" path, but maybe there's > > something I'm missing, and the other patch looks great too. > > > > I didn't read carefully yet (although I noticed two quirks in the first > > patch that I've pointed out below) -- I'll have a closer look once > > you've decided on a direction. > > Thanks. Looks like the clang-format I had installed was a bit too old. > I have also rebased and removed the debug output. > > Cheers, > Rafael > > _______________________________________________ > llvm-commits mailing list > llvm-commits at cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141219/31ad1c5c/attachment.html>
Reasonably Related Threads
- [LLVMdev] opt -O3 causes Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed
- [LLVMdev] opt -O3 causes Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed
- [LLVMdev] opt -O3 causes Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed
- [LLVMdev] opt -O3 causes Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed
- [LLVMdev] memory leaks in *Type::get() and Constant*::get()