Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8393017:
Bitcode streaming (Closed)
|
Created:
9 years, 2 months ago by (google.com) Derek Schuff Modified:
8 years, 9 months ago Reviewers:
nlewycky, pdox, robertm, sehr (please use chromium), jvoung - send to chromium..., jasonwkim CC:
native-client-reviews_googlegroups.com, pnacl-team_google.com Visibility:
Public. |
DescriptionThis patch enables bitcode streaming, behind a flag (-streaming-bitcode).
It is intended to work even if the size of the bitcode is not known in advance, but to work even better if it is known
(a future CL will wrap pnacl bitcode in a header with size info).
* Introduce BitstreamBytes container to hold bitcode in the bitcode reader, instead of a fixed-size memory buffer
* Introduce getStreamedBitcodeModule plumbing, using a callback to fetch more bytes from the stream.
* add BitcodeFileStream to implement the callback for files
* Modify ParseModule to not read/skip the entire bitcode before materialization
Patch Set 1 #
Total comments: 4
Patch Set 2 : Tweak interfaces #Patch Set 3 : rebase #Patch Set 4 : rebase against upstream LLVM #
Total comments: 66
Patch Set 5 : Address comments, incorporate parsing changes from CL 8437024 (still has TODOs) #Patch Set 6 : typo #Patch Set 7 : namespace #Patch Set 8 : rename BitstreamVector and change operator[] to getByte/getWord methods #Patch Set 9 : missed a few #Patch Set 10 : factor cleanups into function #Patch Set 11 : bug #Patch Set 12 : Move files to Support, a few extra cleanups #Patch Set 13 : Rebase against LLVM r144247 #Patch Set 14 : Rebase to 145254 #Patch Set 15 : more comments, rebase to 145582 #Patch Set 16 : put destructors back, fix trailing whitespace #
Messages
Total messages: 19 (0 generated)
One observation on a very high level: why does the reader have to be aware of the two different ways of how it gets its data? Is there way of having an abstract class for this, say: RawCodeStorageAbs Which provides Read() and SkipWrapper, etc. and which can be backed by either a memory buffer or a pipe/lazy loaded file. It would also be nice to have some way of monitoring (heuristic) that the lazy loaded variation does indeed read the file in chunks rather that everything at once.
1 nit and 1 qustion http://codereview.chromium.org/8393017/diff/1/include/llvm/Support/IRReader.h File include/llvm/Support/IRReader.h (right): http://codereview.chromium.org/8393017/diff/1/include/llvm/Support/IRReader.h... include/llvm/Support/IRReader.h:73: // like lazy file module but does not read the whole file at the start; Use /// for doxygen http://codereview.chromium.org/8393017/diff/1/tools/llc/llc.cpp File tools/llc/llc.cpp (left): http://codereview.chromium.org/8393017/diff/1/tools/llc/llc.cpp#oldcode501 tools/llc/llc.cpp:501: PM.run(mod); Why did you expand this?
http://codereview.chromium.org/8393017/diff/1/include/llvm/Support/IRReader.h File include/llvm/Support/IRReader.h (right): http://codereview.chromium.org/8393017/diff/1/include/llvm/Support/IRReader.h... include/llvm/Support/IRReader.h:73: // like lazy file module but does not read the whole file at the start; On 2011/10/25 21:34:12, jasonwkim wrote: > Use /// for doxygen Done. http://codereview.chromium.org/8393017/diff/1/tools/llc/llc.cpp File tools/llc/llc.cpp (left): http://codereview.chromium.org/8393017/diff/1/tools/llc/llc.cpp#oldcode501 tools/llc/llc.cpp:501: PM.run(mod); On 2011/10/25 21:34:12, jasonwkim wrote: > Why did you expand this? Because the module-level pass manager caused the whole bitcode file to be materialized at startup (i think it was just so it could use the iterator over the functions but now I can't find exactly where that was; i'll look again later).
On 2011/10/25 21:19:44, robertm wrote: > One observation on a very high level: > > why does the reader have to be aware of the two different ways of how it gets > its data? The idea is that the bitcode reader does not have to be aware. it is sort of this way now, as the reading is encapsulated in the BitstreamReader/BitstreamCursor classes. BitstreamReader was written to be backed by a MemoryBuffer which has all the bitcode in it. this change introduces BitstreamVector as the interface to BitstreamReader. Ultimately something has to know how to get more bits, but it seems like it's best not to put that into BitstreamVector itself; hence the StreamChunkCallback interface. So the idea is that after this change, only BitstreamVector knows how to use StreamChunkCallback. The other issue here is the plumbing on how to get the callback into the right place; that's the IRReader/ReaderWriter changes. Ultimately llc is what instantiates BitcodeFileStream; and there could be BitcodeHTTPStream, BitcodePipeStream, etc. I'm open to suggestions as to how that plumbing should work though. > It would also be nice to have some way of monitoring (heuristic) that > the lazy loaded variation does indeed read the file in chunks rather that > everything at once. It isn't yet, there are still a few bits that need to be fixed. The simple way I've been using for testing is just putting a printf inside the stream loading callback and in llc after module setup but before compilation.
This looks like a huge # of localmods? Can you use the: // @LOCALMOD and // @LOCALMOD-BEGIN ... // @LOCALMOD-END patterns?
My goal is to try to send this upstream. So once I get some feedback from you guys and make it more complete (i.e. find the places where the bitcode is still being read too eagerly) I will go to nick lewycky or to the LLVM list and have them review it as well. right now the change to our version of llc is kind of temporary; I need to add a commandline flag, etc. and see if people want to replace the existing "lazy" bitcode pieces that aren't really all that lazy, or to add this as something additional. so the highest-level stuff is the least complete/thought-out so far. On Tue, Oct 25, 2011 at 3:38 PM, <pdox@google.com> wrote: > This looks like a huge # of localmods? > > Can you use the: > // @LOCALMOD > > and > > // @LOCALMOD-BEGIN > ... > // @LOCALMOD-END > > patterns? > > > http://codereview.chromium.**org/8393017/<http://codereview.chromium.org/8393... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
On 2011/10/25 22:30:12, Derek Schuff wrote: > On 2011/10/25 21:19:44, robertm wrote: > > One observation on a very high level: > > > > why does the reader have to be aware of the two different ways of how it gets > > its data? > > The idea is that the bitcode reader does not have to be aware. it is sort of > this way now, as the reading is encapsulated in the > BitstreamReader/BitstreamCursor classes. BitstreamReader was written to be > backed by a MemoryBuffer which has all the bitcode in it. this change introduces > BitstreamVector as the interface to BitstreamReader. Ultimately something has to > know how to get more bits, but it seems like it's best not to put that into > BitstreamVector itself; hence the StreamChunkCallback interface. So the idea is > that after this change, only BitstreamVector knows how to use > StreamChunkCallback. > The other issue here is the plumbing on how to get the callback into the right > place; that's the IRReader/ReaderWriter changes. Ultimately llc is what > instantiates BitcodeFileStream; and there could be BitcodeHTTPStream, > BitcodePipeStream, etc. > I'm open to suggestions as to how that plumbing should work though. > Thanks for the clarification - I'll have another look. It might be a good idea to make this write-up into a little design-doc which might also help with the upstreaming. > > It would also be nice to have some way of monitoring (heuristic) that > > the lazy loaded variation does indeed read the file in chunks rather that > > everything at once. > > It isn't yet, there are still a few bits that need to be fixed. > The simple way I've been using for testing is just putting a printf inside the > stream loading callback and in llc after module setup but before compilation.
Derek, Upstreaming is hard, especially something of this magnitude. I'm worried that if you do commit it locally w/o localmod markers, you'll later find you won't be able to upstream it, and it'll remain in our repository without markers.
per our conversation, I will keep it in my local git while I attempt to get it upstream and make any changes (it's not necessary that it go into our repo right away), and if we don't think they will take it upstream, i'll add the LOCALMODs before it goes in here. On Tue, Oct 25, 2011 at 3:55 PM, <pdox@google.com> wrote: > Derek, > > Upstreaming is hard, especially something of this magnitude. I'm worried > that if > you do commit it locally w/o localmod markers, you'll later find you won't > be > able to upstream it, and it'll remain in our repository without markers. > > > http://codereview.chromium.**org/8393017/<http://codereview.chromium.org/8393... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
I'm very concerned that BitcodeStreamer is fully virtual. Most of the time virtual functions aren't that slow, but I expect that you would have to work very hard to defend any usage of any virtual functions in the bitcode reader/writer. Using them for initialization code, but not inside a loop (as in GetBytes) might be reasonable. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... File include/llvm/Bitcode/BitcodeStream.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:1: //===---- llvm/Bitcode/BitcodeStream.h - ----*- C++ -*-===// Missing explanation of the file, also make it exactly 80 columns. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:10: // Support for streaming (lazy reading) of bitcode files on disk End with "." http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:15: #ifndef BITCODESTREAM_H_ Please use LLVM_BITCODE_BITCODESTREAM_H for similarity with neighbouring files. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:18: #include <stddef.h> What do you need this for? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:31: /// Returns true if the streamer owns its backing store (e.g. MemoryBuffer) Newline to separate from above decl http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:32: virtual ~BitcodeStreamer() {} Declare the virtual destructor here, but please it in a .cpp file. The current code causes bloat in every .o file that #includes this header, because there is no key function to base the vtable emission on. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:39: BitcodeStreamer* getBitcodeMemoryBufferStreamer(MemoryBuffer *Buf); "BitcodeStreamer* " vs. "MemoryBuffer *". Please be consistent with where you put the space (before or after the '*' or '&'). Most llvm code puts the space on the left (ala. "MemoryBuffer *"). http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... File include/llvm/Bitcode/BitstreamReader.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:29: class BitstreamVector { This interface doesn't act much like a std::vector. "getEndPos()" is really just vector::size. Maybe we should just rename it "BitstreamBytes"? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:35: // Is Pos the ending file position (one byte past the last valid byte). "Is Pos the ending file position (one byte past the last valid byte)." Huh? Did you mean "Determine whether Pos is the ending file position"? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:41: virtual size_t getEndPos() = 0; Extra space in "() = 0". http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:90: virtual unsigned char operator [](size_t Pos) { This is confusing. Should it instead be returning a reference? Or should the method be const? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:92: && "indexing outside of buffer"); Bad indent. (Can the && go on the previous line? Would it fit? Same as in addressOf). http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:106: LazyBitstreamVector(BitcodeStreamer* streamer) : Bytes(kChunkSize), Please most the constructor list to starting on the following line. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:229: BitstreamReader(BitstreamVector* bsv) { * on the right http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:238: BitstreamVector& getBSV() { return *BSV; } & on the right. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:450: CurWord = (BitStream->getBSV()[NextChar+0] << 0) | Extra space in "<< 0" http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:579: if (CurCodeSize == 0 || AtEndOfStream() || 0) || 0? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:580: // !BitStream->getBSV().canSkipToPos(NextChar+NumWords*4)) Delete commented out code. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... File include/llvm/Bitcode/ReaderWriter.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... include/llvm/Bitcode/ReaderWriter.h:40: /// in *ErrMsg with an error description if ErrMsg is non-null. Reflow text. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... include/llvm/Bitcode/ReaderWriter.h:42: BitcodeStreamer* streamer, Fix indent. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... include/llvm/Bitcode/ReaderWriter.h:126: /// If 'verify' is true, check that the file fits in the buffer. How does the caller tell whether we returned successful because we actually skipped or because it didn't fit? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRRead... File include/llvm/Support/IRReader.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRRead... include/llvm/Support/IRReader.h:24: #include "llvm/Bitcode/BitcodeStream.h" Given that there's no other changes in the file, this can't be right. Either other things we're #include'ing are missing the header, or other things including us are (or neither, and we can just delete this line). See http://llvm.org/docs/CodingStandards.html#hl_dontinclude . http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... File lib/Bitcode/Reader/BitcodeReader.cpp (right): http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:1828: if ((err = InitStream())) return err; if (InitStream()) return true; http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:1940: if ((err = InitStream())) return err; again http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:2925: else return InitStreamFromBuffer(); Delete the "else". See http://llvm.org/docs/CodingStandards.html#hl_else_after_return http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:2960: for (int i = 0; i < 16; i++) buf[i] = BSV->operator[](i); (*BSV)[i] http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:3009: delete M; //Also deletes R. Space after // http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... File lib/Bitcode/Reader/BitcodeReader.h (right): http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.h:131: BitcodeStreamer* LazyStreamer; * on the right http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.h:179: Why the two new blank lines? Please minimize your diff. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.h:186: explicit BitcodeReader(BitcodeStreamer* streamer, LLVMContext &C) * on the right http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... File lib/Bitcode/Reader/BitcodeStream.cpp (right): http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... lib/Bitcode/Reader/BitcodeStream.cpp:1: //===---- llvm/Bitcode/Reader/BitcodeStream.cpp - ----*- C++ -*-===// Missing description, needs to be exactly 80 columns. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... lib/Bitcode/Reader/BitcodeStream.cpp:10: // Support for streaming (lazy reading) of bitcode files on disk Full sentence. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... lib/Bitcode/Reader/BitcodeStream.cpp:62: #ifdef O_BINARY Sorry, code which does this sort of platform-specific thing needs to live in lib/Support. (I note that this seems to be a copy of MemoryBuffer::getFile? Could you refactor?)
On Fri, Nov 4, 2011 at 8:45 PM, <nlewycky@google.com> wrote: > I'm very concerned that BitcodeStreamer is fully virtual. Most of the time > virtual functions aren't that slow, but I expect that you would have to > work > very hard to defend any usage of any virtual functions in the bitcode > reader/writer. Using them for initialization code, but not inside a loop > (as in > GetBytes) might be reasonable. > The design seems much cleaner with this interface being abstract. I am not sure about how often GetBytes is called but I would assume it is linear in the size of the bitcode file with a constant <= 1. So this should result in at most a few million such calls which should not have an performance impact. But let's measure first ;-) > > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h> > File include/llvm/Bitcode/**BitcodeStream.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode1<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode1> > include/llvm/Bitcode/**BitcodeStream.h:1: //===---- > llvm/Bitcode/BitcodeStream.h - ----*- C++ -*-===// > Missing explanation of the file, also make it exactly 80 columns. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode10<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode10> > include/llvm/Bitcode/**BitcodeStream.h:10: // Support for streaming (lazy > reading) of bitcode files on disk > End with "." > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode15<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode15> > include/llvm/Bitcode/**BitcodeStream.h:15: #ifndef BITCODESTREAM_H_ > Please use LLVM_BITCODE_BITCODESTREAM_H for similarity with neighbouring > files. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode18<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode18> > include/llvm/Bitcode/**BitcodeStream.h:18: #include <stddef.h> > What do you need this for? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode31<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode31> > include/llvm/Bitcode/**BitcodeStream.h:31: /// Returns true if the > streamer owns its backing store (e.g. MemoryBuffer) > Newline to separate from above decl > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode32<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode32> > include/llvm/Bitcode/**BitcodeStream.h:32: virtual ~BitcodeStreamer() {} > Declare the virtual destructor here, but please it in a .cpp file. The > current code causes bloat in every .o file that #includes this header, > because there is no key function to base the vtable emission on. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode39<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode39> > include/llvm/Bitcode/**BitcodeStream.h:39: BitcodeStreamer* > getBitcodeMemoryBufferStreamer**(MemoryBuffer *Buf); > "BitcodeStreamer* " vs. "MemoryBuffer *". Please be consistent with > where you put the space (before or after the '*' or '&'). Most llvm code > puts the space on the left (ala. "MemoryBuffer *"). > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h> > File include/llvm/Bitcode/**BitstreamReader.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode29<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode29> > include/llvm/Bitcode/**BitstreamReader.h:29: class BitstreamVector { > This interface doesn't act much like a std::vector. "getEndPos()" is > really just vector::size. Maybe we should just rename it > "BitstreamBytes"? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode35<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode35> > include/llvm/Bitcode/**BitstreamReader.h:35: // Is Pos the ending file > position (one byte past the last valid byte). > "Is Pos the ending file position (one byte past the last valid byte)." > > Huh? Did you mean "Determine whether Pos is the ending file position"? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode41<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode41> > include/llvm/Bitcode/**BitstreamReader.h:41: virtual size_t getEndPos() = > 0; > Extra space in "() = 0". > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode90<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode90> > include/llvm/Bitcode/**BitstreamReader.h:90: virtual unsigned char > operator [](size_t Pos) { > This is confusing. Should it instead be returning a reference? Or should > the method be const? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode92<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode92> > include/llvm/Bitcode/**BitstreamReader.h:92: && "indexing outside of > buffer"); > Bad indent. (Can the && go on the previous line? Would it fit? Same as > in addressOf). > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode106<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode106> > include/llvm/Bitcode/**BitstreamReader.h:106: > LazyBitstreamVector(**BitcodeStreamer* streamer) : Bytes(kChunkSize), > Please most the constructor list to starting on the following line. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode229<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode229> > include/llvm/Bitcode/**BitstreamReader.h:229: > BitstreamReader(**BitstreamVector* bsv) { > * on the right > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode238<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode238> > include/llvm/Bitcode/**BitstreamReader.h:238: BitstreamVector& getBSV() { > return *BSV; } > & on the right. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode450<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode450> > include/llvm/Bitcode/**BitstreamReader.h:450: CurWord = > (BitStream->getBSV()[NextChar+**0] << 0) | > Extra space in "<< 0" > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode579<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode579> > include/llvm/Bitcode/**BitstreamReader.h:579: if (CurCodeSize == 0 || > AtEndOfStream() || 0) > || 0? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode580<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode580> > include/llvm/Bitcode/**BitstreamReader.h:580: // > !BitStream->getBSV().**canSkipToPos(NextChar+**NumWords*4)) > Delete commented out code. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h> > File include/llvm/Bitcode/**ReaderWriter.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h#newcode40<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode40> > include/llvm/Bitcode/**ReaderWriter.h:40: /// in *ErrMsg with an error > description if ErrMsg is non-null. > Reflow text. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h#newcode42<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode42> > include/llvm/Bitcode/**ReaderWriter.h:42: BitcodeStreamer* streamer, > Fix indent. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h#newcode126<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode126> > include/llvm/Bitcode/**ReaderWriter.h:126: /// If 'verify' is true, check > that the file fits in the buffer. > How does the caller tell whether we returned successful because we > actually skipped or because it didn't fit? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Support/IRReader.**h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRReader.h> > File include/llvm/Support/IRReader.**h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Support/IRReader.**h#newcode24<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRReader.h#newcode24> > include/llvm/Support/IRReader.**h:24: #include > "llvm/Bitcode/BitcodeStream.h" > Given that there's no other changes in the file, this can't be right. > Either other things we're #include'ing are missing the header, or other > things including us are (or neither, and we can just delete this line). > > See http://llvm.org/docs/**CodingStandards.html#hl_**dontinclude<http://llvm.org/.... > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp> > File lib/Bitcode/Reader/**BitcodeReader.cpp (right): > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode1828<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode1828> > lib/Bitcode/Reader/**BitcodeReader.cpp:1828: if ((err = InitStream())) > return err; > if (InitStream()) return true; > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode1940<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode1940> > lib/Bitcode/Reader/**BitcodeReader.cpp:1940: if ((err = InitStream())) > return err; > again > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode2925<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode2925> > lib/Bitcode/Reader/**BitcodeReader.cpp:2925: else return > InitStreamFromBuffer(); > Delete the "else". See > http://llvm.org/docs/**CodingStandards.html#hl_else_**after_return<http://llv... > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode2960<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode2960> > lib/Bitcode/Reader/**BitcodeReader.cpp:2960: for (int i = 0; i < 16; i++) > buf[i] = BSV->operator[](i); > (*BSV)[i] > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode3009<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode3009> > lib/Bitcode/Reader/**BitcodeReader.cpp:3009: delete M; //Also deletes R. > Space after // > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.h<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h> > File lib/Bitcode/Reader/**BitcodeReader.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**h#newcode131<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode131> > lib/Bitcode/Reader/**BitcodeReader.h:131: BitcodeStreamer* LazyStreamer; > * on the right > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**h#newcode179<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode179> > lib/Bitcode/Reader/**BitcodeReader.h:179: > Why the two new blank lines? Please minimize your diff. > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**h#newcode186<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode186> > lib/Bitcode/Reader/**BitcodeReader.h:186: explicit > BitcodeReader(BitcodeStreamer* streamer, LLVMContext &C) > * on the right > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp> > File lib/Bitcode/Reader/**BitcodeStream.cpp (right): > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp#newcode1<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode1> > lib/Bitcode/Reader/**BitcodeStream.cpp:1: //===---- > llvm/Bitcode/Reader/**BitcodeStream.cpp - ----*- C++ -*-===// > Missing description, needs to be exactly 80 columns. > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp#newcode10<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode10> > lib/Bitcode/Reader/**BitcodeStream.cpp:10: // Support for streaming (lazy > reading) of bitcode files on disk > Full sentence. > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp#newcode62<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode62> > lib/Bitcode/Reader/**BitcodeStream.cpp:62: #ifdef O_BINARY > Sorry, code which does this sort of platform-specific thing needs to > live in lib/Support. (I note that this seems to be a copy of > MemoryBuffer::getFile? Could you refactor?) > > http://codereview.chromium.**org/8393017/<http://codereview.chromium.org/8393... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
Technically GetBytes can be called in a loop, but it's called to on chunks at a time (in the current CL it's 4k but I think I'm going to make it 16k). If there's a concern about virtual functions, the concern should be about the methods in BitstreamVector, which is called far more often (e.g. there are several calls in each Read). Since you rightly pointed out that it's not much like a Vector, we might as well make the interface return a whole word rather than a byte at a time (since it's only ever called to get a word anyway), and we could just factor the isEndPos check (BitstreamReader.h:441 into the byte-getting call, which would reduce it to one virtual call per Read. (working on the other comments, revisions forthcoming) On Fri, Nov 4, 2011 at 5:45 PM, <nlewycky@google.com> wrote: > I'm very concerned that BitcodeStreamer is fully virtual. Most of the time > virtual functions aren't that slow, but I expect that you would have to > work > very hard to defend any usage of any virtual functions in the bitcode > reader/writer. Using them for initialization code, but not inside a loop > (as in > GetBytes) might be reasonable. > > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h> > File include/llvm/Bitcode/**BitcodeStream.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode1<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode1> > include/llvm/Bitcode/**BitcodeStream.h:1: //===---- > llvm/Bitcode/BitcodeStream.h - ----*- C++ -*-===// > Missing explanation of the file, also make it exactly 80 columns. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode10<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode10> > include/llvm/Bitcode/**BitcodeStream.h:10: // Support for streaming (lazy > reading) of bitcode files on disk > End with "." > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode15<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode15> > include/llvm/Bitcode/**BitcodeStream.h:15: #ifndef BITCODESTREAM_H_ > Please use LLVM_BITCODE_BITCODESTREAM_H for similarity with neighbouring > files. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode18<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode18> > include/llvm/Bitcode/**BitcodeStream.h:18: #include <stddef.h> > What do you need this for? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode31<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode31> > include/llvm/Bitcode/**BitcodeStream.h:31: /// Returns true if the > streamer owns its backing store (e.g. MemoryBuffer) > Newline to separate from above decl > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode32<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode32> > include/llvm/Bitcode/**BitcodeStream.h:32: virtual ~BitcodeStreamer() {} > Declare the virtual destructor here, but please it in a .cpp file. The > current code causes bloat in every .o file that #includes this header, > because there is no key function to base the vtable emission on. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitcodeStream.h#newcode39<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode39> > include/llvm/Bitcode/**BitcodeStream.h:39: BitcodeStreamer* > getBitcodeMemoryBufferStreamer**(MemoryBuffer *Buf); > "BitcodeStreamer* " vs. "MemoryBuffer *". Please be consistent with > where you put the space (before or after the '*' or '&'). Most llvm code > puts the space on the left (ala. "MemoryBuffer *"). > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h> > File include/llvm/Bitcode/**BitstreamReader.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode29<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode29> > include/llvm/Bitcode/**BitstreamReader.h:29: class BitstreamVector { > This interface doesn't act much like a std::vector. "getEndPos()" is > really just vector::size. Maybe we should just rename it > "BitstreamBytes"? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode35<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode35> > include/llvm/Bitcode/**BitstreamReader.h:35: // Is Pos the ending file > position (one byte past the last valid byte). > "Is Pos the ending file position (one byte past the last valid byte)." > > Huh? Did you mean "Determine whether Pos is the ending file position"? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode41<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode41> > include/llvm/Bitcode/**BitstreamReader.h:41: virtual size_t getEndPos() = > 0; > Extra space in "() = 0". > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode90<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode90> > include/llvm/Bitcode/**BitstreamReader.h:90: virtual unsigned char > operator [](size_t Pos) { > This is confusing. Should it instead be returning a reference? Or should > the method be const? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode92<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode92> > include/llvm/Bitcode/**BitstreamReader.h:92: && "indexing outside of > buffer"); > Bad indent. (Can the && go on the previous line? Would it fit? Same as > in addressOf). > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode106<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode106> > include/llvm/Bitcode/**BitstreamReader.h:106: > LazyBitstreamVector(**BitcodeStreamer* streamer) : Bytes(kChunkSize), > Please most the constructor list to starting on the following line. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode229<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode229> > include/llvm/Bitcode/**BitstreamReader.h:229: > BitstreamReader(**BitstreamVector* bsv) { > * on the right > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode238<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode238> > include/llvm/Bitcode/**BitstreamReader.h:238: BitstreamVector& getBSV() { > return *BSV; } > & on the right. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode450<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode450> > include/llvm/Bitcode/**BitstreamReader.h:450: CurWord = > (BitStream->getBSV()[NextChar+**0] << 0) | > Extra space in "<< 0" > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode579<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode579> > include/llvm/Bitcode/**BitstreamReader.h:579: if (CurCodeSize == 0 || > AtEndOfStream() || 0) > || 0? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**BitstreamReader.h#newcode580<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode580> > include/llvm/Bitcode/**BitstreamReader.h:580: // > !BitStream->getBSV().**canSkipToPos(NextChar+**NumWords*4)) > Delete commented out code. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h> > File include/llvm/Bitcode/**ReaderWriter.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h#newcode40<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode40> > include/llvm/Bitcode/**ReaderWriter.h:40: /// in *ErrMsg with an error > description if ErrMsg is non-null. > Reflow text. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h#newcode42<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode42> > include/llvm/Bitcode/**ReaderWriter.h:42: BitcodeStreamer* streamer, > Fix indent. > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Bitcode/**ReaderWriter.h#newcode126<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode126> > include/llvm/Bitcode/**ReaderWriter.h:126: /// If 'verify' is true, check > that the file fits in the buffer. > How does the caller tell whether we returned successful because we > actually skipped or because it didn't fit? > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Support/IRReader.**h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRReader.h> > File include/llvm/Support/IRReader.**h (right): > > http://codereview.chromium.**org/8393017/diff/10001/** > include/llvm/Support/IRReader.**h#newcode24<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRReader.h#newcode24> > include/llvm/Support/IRReader.**h:24: #include > "llvm/Bitcode/BitcodeStream.h" > Given that there's no other changes in the file, this can't be right. > Either other things we're #include'ing are missing the header, or other > things including us are (or neither, and we can just delete this line). > > See http://llvm.org/docs/**CodingStandards.html#hl_**dontinclude<http://llvm.org/.... > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp> > File lib/Bitcode/Reader/**BitcodeReader.cpp (right): > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode1828<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode1828> > lib/Bitcode/Reader/**BitcodeReader.cpp:1828: if ((err = InitStream())) > return err; > if (InitStream()) return true; > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode1940<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode1940> > lib/Bitcode/Reader/**BitcodeReader.cpp:1940: if ((err = InitStream())) > return err; > again > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode2925<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode2925> > lib/Bitcode/Reader/**BitcodeReader.cpp:2925: else return > InitStreamFromBuffer(); > Delete the "else". See > http://llvm.org/docs/**CodingStandards.html#hl_else_**after_return<http://llv... > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode2960<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode2960> > lib/Bitcode/Reader/**BitcodeReader.cpp:2960: for (int i = 0; i < 16; i++) > buf[i] = BSV->operator[](i); > (*BSV)[i] > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**cpp#newcode3009<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode3009> > lib/Bitcode/Reader/**BitcodeReader.cpp:3009: delete M; //Also deletes R. > Space after // > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.h<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h> > File lib/Bitcode/Reader/**BitcodeReader.h (right): > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**h#newcode131<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode131> > lib/Bitcode/Reader/**BitcodeReader.h:131: BitcodeStreamer* LazyStreamer; > * on the right > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**h#newcode179<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode179> > lib/Bitcode/Reader/**BitcodeReader.h:179: > Why the two new blank lines? Please minimize your diff. > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeReader.**h#newcode186<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode186> > lib/Bitcode/Reader/**BitcodeReader.h:186: explicit > BitcodeReader(BitcodeStreamer* streamer, LLVMContext &C) > * on the right > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp> > File lib/Bitcode/Reader/**BitcodeStream.cpp (right): > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp#newcode1<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode1> > lib/Bitcode/Reader/**BitcodeStream.cpp:1: //===---- > llvm/Bitcode/Reader/**BitcodeStream.cpp - ----*- C++ -*-===// > Missing description, needs to be exactly 80 columns. > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp#newcode10<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode10> > lib/Bitcode/Reader/**BitcodeStream.cpp:10: // Support for streaming (lazy > reading) of bitcode files on disk > Full sentence. > > http://codereview.chromium.**org/8393017/diff/10001/lib/** > Bitcode/Reader/BitcodeStream.**cpp#newcode62<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode62> > lib/Bitcode/Reader/**BitcodeStream.cpp:62: #ifdef O_BINARY > Sorry, code which does this sort of platform-specific thing needs to > live in lib/Support. (I note that this seems to be a copy of > MemoryBuffer::getFile? Could you refactor?) > > http://codereview.chromium.**org/8393017/<http://codereview.chromium.org/8393... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... File include/llvm/Bitcode/BitcodeStream.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:1: //===---- llvm/Bitcode/BitcodeStream.h - ----*- C++ -*-===// OK. I'm a little unclear on exactly what is supposed to go at the very top here, vs down below? is this just an ultra-short summary and a more detailed comment below? On 2011/11/05 00:45:06, nlewycky wrote: > Missing explanation of the file, also make it exactly 80 columns.
started addressing comments (the stuff marked as "done" hasn't been uploaded yet), but I wanted to go ahead and send the question/discussion stuff http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... File include/llvm/Bitcode/BitcodeStream.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:10: // Support for streaming (lazy reading) of bitcode files on disk On 2011/11/05 00:45:06, nlewycky wrote: > End with "." Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:15: #ifndef BITCODESTREAM_H_ On 2011/11/05 00:45:06, nlewycky wrote: > Please use LLVM_BITCODE_BITCODESTREAM_H for similarity with neighbouring files. Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:18: #include <stddef.h> Originally had it for size_t. no longer necessary now that I have <string> On 2011/11/05 00:45:06, nlewycky wrote: > What do you need this for? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:31: /// Returns true if the streamer owns its backing store (e.g. MemoryBuffer) On 2011/11/05 00:45:06, nlewycky wrote: > Newline to separate from above decl Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:32: virtual ~BitcodeStreamer() {} On 2011/11/05 00:45:06, nlewycky wrote: > Declare the virtual destructor here, but please it in a .cpp file. The current > code causes bloat in every .o file that #includes this header, because there is > no key function to base the vtable emission on. Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitcod... include/llvm/Bitcode/BitcodeStream.h:39: BitcodeStreamer* getBitcodeMemoryBufferStreamer(MemoryBuffer *Buf); OK. hopefully I got them all in this pass. On 2011/11/05 00:45:06, nlewycky wrote: > "BitcodeStreamer* " vs. "MemoryBuffer *". Please be consistent with where you > put the space (before or after the '*' or '&'). Most llvm code puts the space on > the left (ala. "MemoryBuffer *"). http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... File lib/Bitcode/Reader/BitcodeStream.cpp (right): http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... lib/Bitcode/Reader/BitcodeStream.cpp:1: //===---- llvm/Bitcode/Reader/BitcodeStream.cpp - ----*- C++ -*-===// On 2011/11/05 00:45:06, nlewycky wrote: > Missing description, needs to be exactly 80 columns. Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... lib/Bitcode/Reader/BitcodeStream.cpp:10: // Support for streaming (lazy reading) of bitcode files on disk On 2011/11/05 00:45:06, nlewycky wrote: > Full sentence. Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeS... lib/Bitcode/Reader/BitcodeStream.cpp:62: #ifdef O_BINARY Actually in that case, probably this whole file should go into Support since pretty much anything it does will probably be platform-specific (other than trivial MemoryBuffer wrappers). Actually I'm not sure that the MemoryBuffer example should stay in here, since it will be dead code. (since we see to want to keep the existing MemoryBuffer-based ways of creating Modules) As for refactoring file opening and reading, I'm not sure it's really enough code to be putting into a general abstraction (if it is, surely there must already be some "open this filename and return an fd" kind of abstraction already in LLVM?) in any case, if I were to create a new itnerface, I assume it would just go in lib/Support, or would it be somewhere more specific? On 2011/11/05 00:45:06, nlewycky wrote: > Sorry, code which does this sort of platform-specific thing needs to live in > lib/Support. (I note that this seems to be a copy of MemoryBuffer::getFile? > Could you refactor?)
oh, one other thing I wanted to add to this. with the change below, we'd be down to one virtual call per Read. For LLC or any application that's doing any kind of compilation, this is a tiny bit of noise compared to the cost of compilation. The tools that might potentially care about the performance of bitcode reading (bcanalyzer, clang AST serialization) don't use Read much, instead they pass the Blobstart/BlobLen arguments to ReadRecord which gets a pointer directly to the bitcode and short-circuits most of those calls. So I doubt it would be a serious issue. On Mon, Nov 7, 2011 at 9:11 AM, Derek Schuff <dschuff@google.com> wrote: > Technically GetBytes can be called in a loop, but it's called to on chunks > at a time (in the current CL it's 4k but I think I'm going to make it 16k). > If there's a concern about virtual functions, the concern should be about > the methods in BitstreamVector, which is called far more often (e.g. there > are several calls in each Read). Since you rightly pointed out that it's > not much like a Vector, we might as well make the interface return a whole > word rather than a byte at a time (since it's only ever called to get a > word anyway), and we could just factor the isEndPos check > (BitstreamReader.h:441 into the byte-getting call, which would reduce it to > one virtual call per Read. > > (working on the other comments, revisions forthcoming) > > > On Fri, Nov 4, 2011 at 5:45 PM, <nlewycky@google.com> wrote: > >> I'm very concerned that BitcodeStreamer is fully virtual. Most of the time >> virtual functions aren't that slow, but I expect that you would have to >> work >> very hard to defend any usage of any virtual functions in the bitcode >> reader/writer. Using them for initialization code, but not inside a loop >> (as in >> GetBytes) might be reasonable. >> >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h> >> File include/llvm/Bitcode/**BitcodeStream.h (right): >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h#newcode1<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode1> >> include/llvm/Bitcode/**BitcodeStream.h:1: //===---- >> llvm/Bitcode/BitcodeStream.h - ----*- C++ -*-===// >> Missing explanation of the file, also make it exactly 80 columns. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h#newcode10<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode10> >> include/llvm/Bitcode/**BitcodeStream.h:10: // Support for streaming (lazy >> reading) of bitcode files on disk >> End with "." >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h#newcode15<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode15> >> include/llvm/Bitcode/**BitcodeStream.h:15: #ifndef BITCODESTREAM_H_ >> Please use LLVM_BITCODE_BITCODESTREAM_H for similarity with neighbouring >> files. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h#newcode18<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode18> >> include/llvm/Bitcode/**BitcodeStream.h:18: #include <stddef.h> >> What do you need this for? >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h#newcode31<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode31> >> include/llvm/Bitcode/**BitcodeStream.h:31: /// Returns true if the >> streamer owns its backing store (e.g. MemoryBuffer) >> Newline to separate from above decl >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h#newcode32<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode32> >> include/llvm/Bitcode/**BitcodeStream.h:32: virtual ~BitcodeStreamer() {} >> Declare the virtual destructor here, but please it in a .cpp file. The >> current code causes bloat in every .o file that #includes this header, >> because there is no key function to base the vtable emission on. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitcodeStream.h#newcode39<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitcodeStream.h#newcode39> >> include/llvm/Bitcode/**BitcodeStream.h:39: BitcodeStreamer* >> getBitcodeMemoryBufferStreamer**(MemoryBuffer *Buf); >> "BitcodeStreamer* " vs. "MemoryBuffer *". Please be consistent with >> where you put the space (before or after the '*' or '&'). Most llvm code >> puts the space on the left (ala. "MemoryBuffer *"). >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h> >> File include/llvm/Bitcode/**BitstreamReader.h (right): >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode29<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode29> >> include/llvm/Bitcode/**BitstreamReader.h:29: class BitstreamVector { >> This interface doesn't act much like a std::vector. "getEndPos()" is >> really just vector::size. Maybe we should just rename it >> "BitstreamBytes"? >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode35<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode35> >> include/llvm/Bitcode/**BitstreamReader.h:35: // Is Pos the ending file >> position (one byte past the last valid byte). >> "Is Pos the ending file position (one byte past the last valid byte)." >> >> Huh? Did you mean "Determine whether Pos is the ending file position"? >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode41<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode41> >> include/llvm/Bitcode/**BitstreamReader.h:41: virtual size_t getEndPos() >> = >> 0; >> Extra space in "() = 0". >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode90<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode90> >> include/llvm/Bitcode/**BitstreamReader.h:90: virtual unsigned char >> operator [](size_t Pos) { >> This is confusing. Should it instead be returning a reference? Or should >> the method be const? >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode92<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode92> >> include/llvm/Bitcode/**BitstreamReader.h:92: && "indexing outside of >> buffer"); >> Bad indent. (Can the && go on the previous line? Would it fit? Same as >> in addressOf). >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode106<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode106> >> include/llvm/Bitcode/**BitstreamReader.h:106: >> LazyBitstreamVector(**BitcodeStreamer* streamer) : Bytes(kChunkSize), >> Please most the constructor list to starting on the following line. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode229<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode229> >> include/llvm/Bitcode/**BitstreamReader.h:229: >> BitstreamReader(**BitstreamVector* bsv) { >> * on the right >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode238<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode238> >> include/llvm/Bitcode/**BitstreamReader.h:238: BitstreamVector& getBSV() { >> return *BSV; } >> & on the right. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode450<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode450> >> include/llvm/Bitcode/**BitstreamReader.h:450: CurWord = >> (BitStream->getBSV()[NextChar+**0] << 0) | >> Extra space in "<< 0" >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode579<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode579> >> include/llvm/Bitcode/**BitstreamReader.h:579: if (CurCodeSize == 0 || >> AtEndOfStream() || 0) >> || 0? >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**BitstreamReader.h#newcode580<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/BitstreamReader.h#newcode580> >> include/llvm/Bitcode/**BitstreamReader.h:580: // >> !BitStream->getBSV().**canSkipToPos(NextChar+**NumWords*4)) >> Delete commented out code. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**ReaderWriter.h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h> >> File include/llvm/Bitcode/**ReaderWriter.h (right): >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**ReaderWriter.h#newcode40<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode40> >> include/llvm/Bitcode/**ReaderWriter.h:40: /// in *ErrMsg with an error >> description if ErrMsg is non-null. >> Reflow text. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**ReaderWriter.h#newcode42<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode42> >> include/llvm/Bitcode/**ReaderWriter.h:42: BitcodeStreamer* streamer, >> Fix indent. >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Bitcode/**ReaderWriter.h#newcode126<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/ReaderWriter.h#newcode126> >> include/llvm/Bitcode/**ReaderWriter.h:126: /// If 'verify' is true, check >> that the file fits in the buffer. >> How does the caller tell whether we returned successful because we >> actually skipped or because it didn't fit? >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Support/IRReader.**h<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRReader.h> >> File include/llvm/Support/IRReader.**h (right): >> >> http://codereview.chromium.**org/8393017/diff/10001/** >> include/llvm/Support/IRReader.**h#newcode24<http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRReader.h#newcode24> >> include/llvm/Support/IRReader.**h:24: #include >> "llvm/Bitcode/BitcodeStream.h" >> Given that there's no other changes in the file, this can't be right. >> Either other things we're #include'ing are missing the header, or other >> things including us are (or neither, and we can just delete this line). >> >> See http://llvm.org/docs/**CodingStandards.html#hl_**dontinclude<http://llvm.org/.... >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**cpp<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp> >> File lib/Bitcode/Reader/**BitcodeReader.cpp (right): >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**cpp#newcode1828<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode1828> >> lib/Bitcode/Reader/**BitcodeReader.cpp:1828: if ((err = InitStream())) >> return err; >> if (InitStream()) return true; >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**cpp#newcode1940<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode1940> >> lib/Bitcode/Reader/**BitcodeReader.cpp:1940: if ((err = InitStream())) >> return err; >> again >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**cpp#newcode2925<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode2925> >> lib/Bitcode/Reader/**BitcodeReader.cpp:2925: else return >> InitStreamFromBuffer(); >> Delete the "else". See >> http://llvm.org/docs/**CodingStandards.html#hl_else_**after_return<http://llv... >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**cpp#newcode2960<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode2960> >> lib/Bitcode/Reader/**BitcodeReader.cpp:2960: for (int i = 0; i < 16; i++) >> buf[i] = BSV->operator[](i); >> (*BSV)[i] >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**cpp#newcode3009<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.cpp#newcode3009> >> lib/Bitcode/Reader/**BitcodeReader.cpp:3009: delete M; //Also deletes R. >> Space after // >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.h<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h> >> File lib/Bitcode/Reader/**BitcodeReader.h (right): >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**h#newcode131<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode131> >> lib/Bitcode/Reader/**BitcodeReader.h:131: BitcodeStreamer* LazyStreamer; >> * on the right >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**h#newcode179<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode179> >> lib/Bitcode/Reader/**BitcodeReader.h:179: >> Why the two new blank lines? Please minimize your diff. >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeReader.**h#newcode186<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeReader.h#newcode186> >> lib/Bitcode/Reader/**BitcodeReader.h:186: explicit >> BitcodeReader(BitcodeStreamer* streamer, LLVMContext &C) >> * on the right >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeStream.**cpp<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp> >> File lib/Bitcode/Reader/**BitcodeStream.cpp (right): >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeStream.**cpp#newcode1<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode1> >> lib/Bitcode/Reader/**BitcodeStream.cpp:1: //===---- >> llvm/Bitcode/Reader/**BitcodeStream.cpp - ----*- C++ -*-===// >> Missing description, needs to be exactly 80 columns. >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeStream.**cpp#newcode10<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode10> >> lib/Bitcode/Reader/**BitcodeStream.cpp:10: // Support for streaming (lazy >> reading) of bitcode files on disk >> Full sentence. >> >> http://codereview.chromium.**org/8393017/diff/10001/lib/** >> Bitcode/Reader/BitcodeStream.**cpp#newcode62<http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeStream.cpp#newcode62> >> lib/Bitcode/Reader/**BitcodeStream.cpp:62: #ifdef O_BINARY >> Sorry, code which does this sort of platform-specific thing needs to >> live in lib/Support. (I note that this seems to be a copy of >> MemoryBuffer::getFile? Could you refactor?) >> >> http://codereview.chromium.**org/8393017/<http://codereview.chromium.org/8393... >> > > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
http://codereview.chromium.org/8393017/diff/10001/tools/llc/llc.cpp File tools/llc/llc.cpp (right): http://codereview.chromium.org/8393017/diff/10001/tools/llc/llc.cpp#newcode365 tools/llc/llc.cpp:365: PassManagerBase *PM; Is there a scoped-pointer class that you can use?
Addressed most of the comments; A couple of clarification questions in there. Also went ahead and added the changes from http://codereview.chromium.org/8437024/ into this CL, since they will have to go in together anyway. Next thing I will do is rename BitstreamVector (and probably change the interface from byte-based to word-based as mentioned in a previous comment... But that discussion is still open) http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... File include/llvm/Bitcode/ReaderWriter.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... include/llvm/Bitcode/ReaderWriter.h:40: /// in *ErrMsg with an error description if ErrMsg is non-null. On 2011/11/05 00:45:06, nlewycky wrote: > Reflow text. Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... include/llvm/Bitcode/ReaderWriter.h:42: BitcodeStreamer* streamer, On 2011/11/05 00:45:06, nlewycky wrote: > Fix indent. Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Reader... include/llvm/Bitcode/ReaderWriter.h:126: /// If 'verify' is true, check that the file fits in the buffer. I'm not sure I understand the question. Before it returned unsuccessful if the buffer wasn't big enough for the header, or if the buffer is not big enough for the header-declared size of the file. Otherwise it returns successful. The new version skips the 2nd buffer size check because the provided buffer does not contain all the bitcode. it only returns unsuccessful if the buffer is not even big enough for the header. On 2011/11/05 00:45:06, nlewycky wrote: > How does the caller tell whether we returned successful because we actually > skipped or because it didn't fit? http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRRead... File include/llvm/Support/IRReader.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Support/IRRead... include/llvm/Support/IRReader.h:24: #include "llvm/Bitcode/BitcodeStream.h" Sorry, this was leftover from a previous iteration where I did change this file. Removed. On 2011/11/05 00:45:06, nlewycky wrote: > Given that there's no other changes in the file, this can't be right. Either > other things we're #include'ing are missing the header, or other things > including us are (or neither, and we can just delete this line). > > See http://llvm.org/docs/CodingStandards.html#hl_dontinclude . http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... File lib/Bitcode/Reader/BitcodeReader.cpp (right): http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:1828: if ((err = InitStream())) return err; On 2011/11/05 00:45:06, nlewycky wrote: > if (InitStream()) return true; Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:1940: if ((err = InitStream())) return err; On 2011/11/05 00:45:06, nlewycky wrote: > again Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:2960: for (int i = 0; i < 16; i++) buf[i] = BSV->operator[](i); On 2011/11/05 00:45:06, nlewycky wrote: > (*BSV)[i] Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.cpp:3009: delete M; //Also deletes R. On 2011/11/05 00:45:06, nlewycky wrote: > Space after // Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... File lib/Bitcode/Reader/BitcodeReader.h (right): http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.h:131: BitcodeStreamer* LazyStreamer; On 2011/11/05 00:45:06, nlewycky wrote: > * on the right Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.h:179: On 2011/11/05 00:45:06, nlewycky wrote: > Why the two new blank lines? Please minimize your diff. Done. http://codereview.chromium.org/8393017/diff/10001/lib/Bitcode/Reader/BitcodeR... lib/Bitcode/Reader/BitcodeReader.h:186: explicit BitcodeReader(BitcodeStreamer* streamer, LLVMContext &C) On 2011/11/05 00:45:06, nlewycky wrote: > * on the right Done. http://codereview.chromium.org/8393017/diff/10001/tools/llc/llc.cpp File tools/llc/llc.cpp (right): http://codereview.chromium.org/8393017/diff/10001/tools/llc/llc.cpp#newcode365 tools/llc/llc.cpp:365: PassManagerBase *PM; On 2011/11/07 19:01:55, jvoung wrote: > Is there a scoped-pointer class that you can use? Done.
OK, I think I have all of the original comments. probably a good time to take another look and/or continue the discussion points already started. I still have a couple TODOs in there. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... File include/llvm/Bitcode/BitstreamReader.h (right): http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:35: // Is Pos the ending file position (one byte past the last valid byte). On 2011/11/05 00:45:06, nlewycky wrote: > "Is Pos the ending file position (one byte past the last valid byte)." > > Huh? Did you mean "Determine whether Pos is the ending file position"? Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:41: virtual size_t getEndPos() = 0; On 2011/11/05 00:45:06, nlewycky wrote: > Extra space in "() = 0". Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:90: virtual unsigned char operator [](size_t Pos) { This container isn't supposed to be mutable. also, any of these functions can require the streaming version to fetch more bytes. On 2011/11/05 00:45:06, nlewycky wrote: > This is confusing. Should it instead be returning a reference? Or should the > method be const? Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:92: && "indexing outside of buffer"); On 2011/11/05 00:45:06, nlewycky wrote: > Bad indent. (Can the && go on the previous line? Would it fit? Same as in > addressOf). Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:106: LazyBitstreamVector(BitcodeStreamer* streamer) : Bytes(kChunkSize), On 2011/11/05 00:45:06, nlewycky wrote: > Please most the constructor list to starting on the following line. Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:229: BitstreamReader(BitstreamVector* bsv) { On 2011/11/05 00:45:06, nlewycky wrote: > * on the right Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:238: BitstreamVector& getBSV() { return *BSV; } On 2011/11/05 00:45:06, nlewycky wrote: > & on the right. Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:450: CurWord = (BitStream->getBSV()[NextChar+0] << 0) | On 2011/11/05 00:45:06, nlewycky wrote: > Extra space in "<< 0" Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:579: if (CurCodeSize == 0 || AtEndOfStream() || 0) On 2011/11/05 00:45:06, nlewycky wrote: > || 0? Done. http://codereview.chromium.org/8393017/diff/10001/include/llvm/Bitcode/Bitstr... include/llvm/Bitcode/BitstreamReader.h:580: // !BitStream->getBSV().canSkipToPos(NextChar+NumWords*4)) On 2011/11/05 00:45:06, nlewycky wrote: > Delete commented out code. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
