Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14813032:
Make abbreviations explicit in pnacl-freeze/thaw. (Closed)
|
Created:
7 years, 7 months ago by Karl Modified:
7 years, 7 months ago CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master Visibility:
Public. |
DescriptionMake abbreviations explicit in pnacl-freeze/thaw.
[1] Explicitly enumerate all abbreviation values, including the
maximum abbreviation for each type of block.
[2] Make "enter subblock" calculate number of bits needed by passing
in maximum abbreviation (associated with block) rather than requiring
the developer to compute this value every time a subblock is entered.
*NOTE* This code changes encoding sizes to be based on
the maximum allowed value, rather than requiring the
developer to calculate out the number of bits needed. This
change doesn't make the PNaCL bitcode files
incompatable with LLVM bitcode files, since it does
not effect the bitcode reader.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3405
R=jvoung@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=80b7ba7
Patch Set 1 #Patch Set 2 : Fix typos in CL #
Total comments: 46
Patch Set 3 : Move abbreviations to global level and factor out code. #Patch Set 4 : Remove dead code. #
Total comments: 33
Patch Set 5 : Fix code based on Jan's suggestions. #
Total comments: 17
Patch Set 6 : Fix issues raised by Jan. #Patch Set 7 : Add more brief comments. #
Total comments: 6
Patch Set 8 : Small cleanups suggested by Jan in CL. #
Messages
Total messages: 12 (0 generated)
PTAL. Thanks.
Still looking through, and trying to understand the change, but here are some comments. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:52: // In Addition, we assume up to two additional enumerated constants are In addition, ... https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:235: // AllCutoff. what is AllCutoff? https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:451: int32_t ReadIntVBR(unsigned NumBits) { Can we just use BitcodeReader::decodeSignRotatedValue(Read...()) ? https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:23: #include "llvm/Support/raw_ostream.h" what is this used for? https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:204: void EmitIntVBR(int32_t Val, unsigned NumBits) { There is static void emitSignedInt64(SmallVectorImpl<uint64_t> &Vals, uint64_t V) { ... } In lib/Bitcode/Writer/BitcodeWriter.cpp. Seems best to keep the way signed VBR values are emitted consistent. I think all they do there, is move the sign bit to the back, then use the original EmitVBR. That seems simpler than trying to set up EmitIntVBRChunk, etc.? https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:265: EmitIntVBR((CodeLen.UseFixed() How come this can be a negative value: -CodeLen.UseNumBits()? https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClDebugging.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClDebugging.h:14: * To turn on debugging of instruction decoding, change value of We shouldn't need our own debugging utils? llvm has Debug.h, which allows you to specify: -debug (all debug output) -debug-only=module_foo (only debug output from module_foo) So, you can get file-level debugging with -debug-only=... Using the commandline switch means that you don't need to recompile to get debugging output. The debug code is also left out when building in Release mode. See the DEBUG_TYPE macro and : http://llvm.org/docs/doxygen/html/Debug_8h.html#ad78e062f62e0d6e453941fb4ca84... https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:554: if (MODULE_GLOBALVAR_ABBREV != SimpleGVarAbbrev) to be consistent with the other refactorings, you could get rid of the SimpleGVarAbbrev variable... https://codereview.chromium.org/14813032/diff/2001/tools/pnacl-bcanalyzer/pna... File tools/pnacl-bcanalyzer/pnacl-bcanalyzer.cpp (right): https://codereview.chromium.org/14813032/diff/2001/tools/pnacl-bcanalyzer/pna... tools/pnacl-bcanalyzer/pnacl-bcanalyzer.cpp:333: NACL_DEBUG(outs() << Indent << "-> ParseBlock(" << BlockID << ")\n"); Not sure you want to use "outs()" since that will mix your debug output with the stdout output. You could use dbgs() instead, to print to stderr.
PTAL. Thanks. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:23: #include "llvm/Support/ErrorHandling.h" Added '#include "llvm/Support/MathExtras.h" so that we can use Log2_32_Ceil. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:52: // In Addition, we assume up to two additional enumerated constants are On 2013/05/14 23:47:14, jvoung (cr) wrote: > In addition, ... Done. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:207: unsigned NumBits; Made publicly visible so that it can be set by the reader. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:209: bool UseFixedAbbrev; Renamed to IsFixed, and made publicly visible (so that it can be set by the reader). https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:214: static unsigned BitsNeededForValue(unsigned Value) { Moved this out (above) to be an inline. Also added inline routines NaClEncodeSignRotatedValue NaClDecodeSignRotatedValue so that they can be (consistently) used. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:215: unsigned NumBits = 1; Replaced body with call to Log2_32_Ceil. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:235: // AllCutoff. On 2013/05/14 23:47:14, jvoung (cr) wrote: > what is AllCutoff? Fixed reference to use "MaxAbbrev". https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitCodes.h:256: bool UseFixed() const { Removed UseFixed and UseNumBits, since the fields are now public and can be changed. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:189: int32_t CurCodeSize; Changed to be an instance of NaClBitcodeSelectorAbrev. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:195: int32_t PrevCodeSize; Changed to be an instance of NaClBitcodeSelectorAbbrev. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:205: int32_t SignedValue(uint32_t Val, bool IsNegative) { Removed, no longer used. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:264: unsigned getAbbrevIDWidth() const { return CurCodeSize; } return CurCodeSize -> return CurCodeSize.NumBits https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:451: int32_t ReadIntVBR(unsigned NumBits) { On 2013/05/14 23:47:14, jvoung (cr) wrote: > Can we just use BitcodeReader::decodeSignRotatedValue(Read...()) ? Removed. Use NaClDecodeSignRotatedValue. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:490: return (CurCodeSize < 0) ? ReadVBR(-CurCodeSize) : Read(CurCodeSize); Changed to: return CurCodeSize.IsFixed ? Read(CurCodeSize.NumBits) : ReadVBR(CurCodeSize.NumBits); https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:23: #include "llvm/Support/raw_ostream.h" On 2013/05/14 23:47:14, jvoung (cr) wrote: > what is this used for? Removed. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:100: void EmitIntVBRChunk(uint32_t Val, bool IsNegative, unsigned NumBits, Removed. No longer used. Uses NaClEncodeSignRotatedValue to convert integer to unsigned integer. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:204: void EmitIntVBR(int32_t Val, unsigned NumBits) { On 2013/05/14 23:47:14, jvoung (cr) wrote: > There is static void emitSignedInt64(SmallVectorImpl<uint64_t> &Vals, uint64_t > V) { ... } > > In lib/Bitcode/Writer/BitcodeWriter.cpp. > > Seems best to keep the way signed VBR values are emitted consistent. > > I think all they do there, is move the sign bit to the back, then use the > original EmitVBR. That seems simpler than trying to set up EmitIntVBRChunk, > etc.? Removed. No longer used. Uses NaClEncodeSignRotatedValue to convert integer to unsigned integer. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:233: if (CurCodeSize.UseFixed()) Modified code to use fields since methods UseFixed and UseNuMBits no longer exist. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:265: EmitIntVBR((CodeLen.UseFixed() On 2013/05/14 23:47:14, jvoung (cr) wrote: > How come this can be a negative value: -CodeLen.UseNumBits()? I modified this to be more explicit using: EmitBool(CodeLen.IsFixed); EmitVBR(CodeLen.NumBits, naclbitc::CodeLenWidth); Changing this to be more explicit and do the FIXED/VBR as a separate check. https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... File include/llvm/Bitcode/NaCl/NaClDebugging.h (right): https://codereview.chromium.org/14813032/diff/2001/include/llvm/Bitcode/NaCl/... include/llvm/Bitcode/NaCl/NaClDebugging.h:14: * To turn on debugging of instruction decoding, change value of On 2013/05/14 23:47:14, jvoung (cr) wrote: > We shouldn't need our own debugging utils? > > llvm has Debug.h, which allows you to specify: > > -debug (all debug output) > -debug-only=module_foo (only debug output from module_foo) > > So, you can get file-level debugging with -debug-only=... > > Using the commandline switch means that you don't need to recompile to get > debugging output. > > The debug code is also left out when building in Release mode. > > See the DEBUG_TYPE macro and : > http://llvm.org/docs/doxygen/html/Debug_8h.html#ad78e062f62e0d6e453941fb4ca84... Removed and using llvm debug.h https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:26: #include "llvm/Support/raw_ostream.h" Removed include, no longer needed. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:29: /* To turn on debugging for this file, change value of NACL_DEBUGGING to 1. Replaced with builtin DEBUG of llvm. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:982: uint64_t NaClBitcodeReader::decodeSignRotatedValue(uint64_t V) { Lifted this definition to include file "llvm/Bitcode/NaCl/NaClBitCodes.h" which names this function as NaClDecodeSignRotatedValue. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1032: NaClBitcodeReader::decodeSignRotatedValue); Replaced with call to NaClDecodeRotatedValue. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1090: V = ConstantInt::get(CurTy, decodeSignRotatedValue(Record[0])); decodeSignRotatedValue -> NaClDecodeSignRotatedValue https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right): https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:74: CurCodeSize = ReadIntVBR(naclbitc::CodeLenWidth); Changed this to: CurCodeSize.IsFixed = ReadBool(); CurCodeSize.NumBits = ReadVBR(naclbitc::CodeLenWidth); https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Reader/Na... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:80: if (CurCodeSize == 0 || AtEndOfStream()) CurCodeSize -> CurCodeSize.NumBits https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:35: /* To turn on debugging for this file, change value of NACL_DEBUGGING to 1. Replaced this with llvm's DEBUG. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:554: if (MODULE_GLOBALVAR_ABBREV != SimpleGVarAbbrev) On 2013/05/14 23:47:14, jvoung (cr) wrote: > to be consistent with the other refactorings, you could get rid of the > SimpleGVarAbbrev variable... Added this to all levels, so that abbreviations can be applied everywhere. However, we do a separate abbreviation for each call, since the abbreviation is optimized on alignment and if it has a section map. Results in smaller bitcode files, as well as removing the need for SimpleVVarAbbrev. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:696: // Abbrev for METADATA_STRING. Moved this local abbreviation to the "global" creation of abbreviations, guaranteeing that this abbreviation will always be used. The previous code would sometimes abbreviate, and sometimes not, depending on the first (interesting) entry in the for loop. For many examples I tried, this saved considerable space by always defining. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:826: static void emitSignedInt64(SmallVectorImpl<uint64_t> &Vals, uint64_t V) { Rewrote this to use NaClEncodeSignRotatedValue defined in NaClBitCodes.h https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:872: unsigned AggregateAbbrev = 0; Note that the constant abbreviations (or at least for CST_CODE_AGGREGATE), they are dependent on argument values. Making these global (and generalizing size) appears to make bitcode files significantly larger, so I have not moved these encodings to be global. Added comment in code to state this. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1727: Log2_32_Ceil(VE.getTypes().size()+1))); Modified to use NaClBitsNeededForValue. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1747: Log2_32_Ceil(VE.getTypes().size()+1))); Modified to use NaClBitsNeededForValue. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1800: Log2_32_Ceil(VE.getTypes().size()+1))); Modified to use NaClBitsNeededForValue. https://codereview.chromium.org/14813032/diff/2001/lib/Bitcode/NaCl/Writer/Na... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1828: } Added Abbreviations for METADATA_STRING, defining it as a global abbreviation. https://codereview.chromium.org/14813032/diff/2001/tools/pnacl-bcanalyzer/pna... File tools/pnacl-bcanalyzer/pnacl-bcanalyzer.cpp (right): https://codereview.chromium.org/14813032/diff/2001/tools/pnacl-bcanalyzer/pna... tools/pnacl-bcanalyzer/pnacl-bcanalyzer.cpp:333: NACL_DEBUG(outs() << Indent << "-> ParseBlock(" << BlockID << ")\n"); On 2013/05/14 23:47:14, jvoung (cr) wrote: > Not sure you want to use "outs()" since that will mix your debug output with the > stdout output. You could use dbgs() instead, to print to stderr. Fixing to use llvm debug.h
https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (left): https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:20: Leave this newline in between the ifndef and the first include. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:66: DEFAULT_MAX_ABBREV = FIRST_APPLICATION_ABBREV-1 I see, so this is supposed to match the hardcoded "3" values? The hardcoded "3" were actually conservative since the hardcoded values were supposed to be LOG2_32_Ceil(3), aka 2? Please mention in the commit message anything that makes us incompatible with upstream... (since that seems big) https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:211: /// encodeSignRotatedValue - Encode a signed value by moving the sign method name mismatch. To avoid this mismatch in documentation of method name and the actual method name, I think the new style of documentation for LLVM is to just use doxygen markup and avoid having to type out the method name: "/// \brief text that is a brief description for foo /// ... void foo(); " http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments === Is there really a need to prefix the function name with NaCl? This is in namespace naclbitc? === Also... turns out functions should start with lower case: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-... I know a lot of the bitcode reader functions don't do that. Could fix in a separate CL. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:250: NaClBitcodeSelectorAbbrev(unsigned MaxAbbrev) Seems like this would be less "magic", but more code, if it was an "explicit" constructor. Your EnterSubblock already create these this explicitly from "unsigned" params. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:228: EmitBool(CodeLen.IsFixed); See below: if the fixedabbrevcutoff variant isn't used, then there's no need to deviate from upstream and end up with an extra EmitBool here? https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:271: void EnterSubblock(unsigned BlockId, Is this actually used? https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1535: dbgs() << "Skip unknown context\n"; Put this under DEBUG() also https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1576: DEBUG(dbgs() << "<- ParseModule\n"); nit: Is it necessary to print the debug "<-" in the return true cases (error cases)? https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (left): https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:491: if (!M->global_empty()) { Why delete the predicate for global_empty()? https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:653: Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::METADATA_STRING)); why is this now deleted instead of emitting an abbrev? https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:39: /// be kept in sync with the reader, but need to be consistent within this file. Perhaps document that for each block type, there should be a GRP_MAX_ABBREV = GRP_LAST_ABBEV. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:236: DEBUG(dbgs() << "<- WriteAbbributeGroupTable\n");} nit: keep the last curly on a separate line. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:698: // Stream.EmitRecord(naclbitc::METADATA_STRING, Record, MDSAbbrev); delete commented out? https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:699: Stream.EmitRecord(naclbitc::METADATA_STRING, Record, METADATA_STRING_ABBREV); 80 col https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1824: NaClBitCodeAbbrev *Abbv = new NaClBitCodeAbbrev(); why is this moved into the function block?
PTAL. Thanks. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (left): https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:20: On 2013/05/17 23:40:48, jvoung (cr) wrote: > Leave this newline in between the ifndef and the first include. Done. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:66: DEFAULT_MAX_ABBREV = FIRST_APPLICATION_ABBREV-1 On 2013/05/17 23:40:48, jvoung (cr) wrote: > I see, so this is supposed to match the hardcoded "3" values? The hardcoded "3" > were actually conservative since the hardcoded values were supposed to be > LOG2_32_Ceil(3), aka 2? > > Please mention in the commit message anything that makes us incompatible with > upstream... (since that seems big) Yes, they should have used log32_32_Ceil(3) rather than 3. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:211: /// encodeSignRotatedValue - Encode a signed value by moving the sign On 2013/05/17 23:40:48, jvoung (cr) wrote: > method name mismatch. > > To avoid this mismatch in documentation of method name and the actual method > name, I think the new style of documentation for LLVM is to just use doxygen > markup and avoid having to type out the method name: > > "/// \brief text that is a brief description for foo > /// ... > void foo(); > " > > http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments > > === > > Is there really a need to prefix the function name with NaCl? This is in > namespace naclbitc? > > === > > Also... turns out functions should start with lower case: > http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-... > > I know a lot of the bitcode reader functions don't do that. Could fix in a > separate CL. Good to know. Will fix the obvious cases, and leave the reast to a separate CL. BTW, the prefix NaCl is needed because all code below initial enums are in namespace llvm. This follows the corresponding namespace structure of BitCodes.h https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:250: NaClBitcodeSelectorAbbrev(unsigned MaxAbbrev) On 2013/05/17 23:40:48, jvoung (cr) wrote: > Seems like this would be less "magic", but more code, if it was an "explicit" > constructor. Your EnterSubblock already create these this explicitly from > "unsigned" params. Made explicit. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:141: void EmitBool(bool b) { Deleting this, since it is no longer needed. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:228: EmitBool(CodeLen.IsFixed); On 2013/05/17 23:40:48, jvoung (cr) wrote: > See below: if the fixedabbrevcutoff variant isn't used, then there's no need to > deviate from upstream and end up with an extra EmitBool here? Deleted. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:271: void EnterSubblock(unsigned BlockId, On 2013/05/17 23:40:48, jvoung (cr) wrote: > Is this actually used? Again, I'm over planning changes. Removing this case, since it is not yet necessary. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1535: dbgs() << "Skip unknown context\n"; On 2013/05/17 23:40:48, jvoung (cr) wrote: > Put this under DEBUG() also Done. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1576: DEBUG(dbgs() << "<- ParseModule\n"); On 2013/05/17 23:40:48, jvoung (cr) wrote: > nit: Is it necessary to print the debug "<-" in the return true cases (error > cases)? Good point. Did this in some places, but not everywhere. Fixing to not generate on error cases. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right): https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Reader/N... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:74: CurCodeSize.IsFixed = ReadBool(); Fixed to always set field IsFixed to true. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (left): https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:491: if (!M->global_empty()) { On 2013/05/17 23:40:48, jvoung (cr) wrote: > Why delete the predicate for global_empty()? I did this because these abbreviations appear commonly in non-global cases, and the resulting bitcode files are significantly smaller. However, the abbreviation can't be made globa because the abbreviations are dependent on locals MaxGlobalType, MaxEncAlignment, and SectionMap.size(). https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:653: Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::METADATA_STRING)); On 2013/05/17 23:40:48, jvoung (cr) wrote: > why is this now deleted instead of emitting an abbrev? This was added as an abbreviation in the "BlockInfo" block of the module, making it apply everywhere. The current code only applies the abbreviation if a MDString occurs before an NDNode (see loop above). From the cases I looked at, this is a bad assumption. If we lift it to a global abbreviation, it applies in many more cases, shrinking the generated bitcode size. Note: This encoding simplification, the global module info abbreviations, and the METADATA string abbreviation result in bitcode files that are about 0.6% smaller. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:39: /// be kept in sync with the reader, but need to be consistent within this file. On 2013/05/17 23:40:48, jvoung (cr) wrote: > Perhaps document that for each block type, there should be a GRP_MAX_ABBREV = > GRP_LAST_ABBEV. Done. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:236: DEBUG(dbgs() << "<- WriteAbbributeGroupTable\n");} On 2013/05/17 23:40:48, jvoung (cr) wrote: > nit: keep the last curly on a separate line. Done. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:698: // Stream.EmitRecord(naclbitc::METADATA_STRING, Record, MDSAbbrev); On 2013/05/17 23:40:48, jvoung (cr) wrote: > delete commented out? Done. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:699: Stream.EmitRecord(naclbitc::METADATA_STRING, Record, METADATA_STRING_ABBREV); On 2013/05/17 23:40:48, jvoung (cr) wrote: > 80 col Done. https://codereview.chromium.org/14813032/diff/16001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1824: NaClBitCodeAbbrev *Abbv = new NaClBitCodeAbbrev(); On 2013/05/17 23:40:48, jvoung (cr) wrote: > why is this moved into the function block? This isn't in the the function block. It is the BlockInfo block, which holds abbreviations that apply to all occurrences of a block. Note that it adds VST_ENTRY, CONSTANTS_BLOCK, and FUNCTION_BLOCK abbreviations.
Not quite sure about the metadata string part, I'll make another pass to see if I understand it... or maybe we can discuss in person. https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/14813032/diff/16001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:66: DEFAULT_MAX_ABBREV = FIRST_APPLICATION_ABBREV-1 On 2013/05/20 22:59:20, Karl wrote: > On 2013/05/17 23:40:48, jvoung (cr) wrote: > > I see, so this is supposed to match the hardcoded "3" values? The hardcoded > "3" > > were actually conservative since the hardcoded values were supposed to be > > LOG2_32_Ceil(3), aka 2? > > > > Please mention in the commit message anything that makes us incompatible with > > upstream... (since that seems big) > > Yes, they should have used log32_32_Ceil(3) rather than 3. > Actually, is it really an incompatibility, or just a difference? Doesn't the reader just read in the abbrev and use that to determine the number of bits? https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:218: // decodeSignRotatedValue - Decode a signed value stored with the sign bit in NaClDecodeSignRotatedValue, or just use "\brief" instead of typing out the method name https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:258: NaClBitcodeSelectorAbbrev(unsigned FirstChunkCutoff, unsigned MaxAbbrev) firstchunkcutoff version not used yet here also https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:443: return (uint32_t) NaClDecodeSignRotatedValue(ReadVBR64(NumBits)); doesn't look like this readintvbr is used https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:92: METADATA_STRING_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV, see below, is this still part of the META_DATA_BLOCK? https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:564: // Don't bother emitting vis + thread local. Can this comment be moved back into the globalvar abbrev code block above? If it is thread-local, it looks like there is no abbrev used below, so the comment is more about the abbrev structure? https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:694: METADATA_MAX_ABBREV); If I'm understanding this correctly, ... if the abbrev for METADATA_STRING is moved out to the top level blockinfo, should the max be calculated differently? https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:811: Stream.EnterSubblock(naclbitc::METADATA_BLOCK_ID); Consistency with other entersubblock METADATA_BLOCK_ID?
PTAL. Thanks. https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:218: // decodeSignRotatedValue - Decode a signed value stored with the sign bit in On 2013/05/21 17:24:17, jvoung (cr) wrote: > NaClDecodeSignRotatedValue, or just use "\brief" instead of typing out the > method name Done. https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitCodes.h:258: NaClBitcodeSelectorAbbrev(unsigned FirstChunkCutoff, unsigned MaxAbbrev) On 2013/05/21 17:24:17, jvoung (cr) wrote: > firstchunkcutoff version not used yet here also Removed. https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:443: return (uint32_t) NaClDecodeSignRotatedValue(ReadVBR64(NumBits)); On 2013/05/21 17:24:17, jvoung (cr) wrote: > doesn't look like this readintvbr is used Removed. https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:219: const NaClBitcodeSelectorAbbrev& CodeLen) { Refactored the private version of this function to be passed the BlockInfo pointer. This allows EnterSubBlock() to get the BlockInfo pointer and define the code length based on the number of abbreviations in the blockinfo structure. The (new) public version calls the private version by simply getting the BlockInfo pointer and passing it in. https://codereview.chromium.org/14813032/diff/27001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:253: void EnterSubblock(unsigned BlockID) { Modified this function to calculate the code length based on the number of abbreviations defined in the BlockInfo. https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:92: METADATA_STRING_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV, On 2013/05/21 17:24:17, jvoung (cr) wrote: > see below, is this still part of the META_DATA_BLOCK? Yes. Its added to the (global) BlockInfo vector via the call to WriteBlockInfo. https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:564: // Don't bother emitting vis + thread local. On 2013/05/21 17:24:17, jvoung (cr) wrote: > Can this comment be moved back into the globalvar abbrev code block above? > > If it is thread-local, it looks like there is no abbrev used below, so the > comment is more about the abbrev structure? Done. https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:694: METADATA_MAX_ABBREV); On 2013/05/21 17:24:17, jvoung (cr) wrote: > If I'm understanding this correctly, ... if the abbrev for METADATA_STRING is > moved out to the top level blockinfo, should the max be calculated differently? > I'm not sure what you are asking. The call to EnterSubblock needs to know what the maximum value is, so that it can calculate the number of bits. The maximum for a METADATA_BLOCK_ID is METADATA_STRING_ABBREV, which is the value set to METADATA_MAX_ABBREV. Are you asking that we get the values from the abbereviations currently stored. We could do this for the default argument form for EnterSubblock. Thinking about this some more, that appears to be the better solution. That will simplify the code for all cases except where local abbreviations need to be added. Applying simplification to EnterSubblock. https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:811: Stream.EnterSubblock(naclbitc::METADATA_BLOCK_ID); On 2013/05/21 17:24:17, jvoung (cr) wrote: > Consistency with other entersubblock METADATA_BLOCK_ID? Done.
LGTM https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/27001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:694: METADATA_MAX_ABBREV); On 2013/05/21 23:06:52, Karl wrote: > On 2013/05/21 17:24:17, jvoung (cr) wrote: > > If I'm understanding this correctly, ... if the abbrev for METADATA_STRING is > > moved out to the top level blockinfo, should the max be calculated > differently? > > > > I'm not sure what you are asking. The call to EnterSubblock needs to know what > the maximum value is, so that it can calculate the number of bits. The maximum > for a METADATA_BLOCK_ID is METADATA_STRING_ABBREV, which is the value set to > METADATA_MAX_ABBREV. > > Are you asking that we get the values from the abbereviations currently stored. > We could do this for the default argument form for EnterSubblock. > > Thinking about this some more, that appears to be the better solution. That will > simplify the code for all cases except where local abbreviations need to be > added. Applying simplification to EnterSubblock. I just wasn't quite understanding how BlockInfos worked. Yeah, it might be better to do it automatically unless there are local abbreviations. https://codereview.chromium.org/14813032/diff/42001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/14813032/diff/42001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:263: /// (global) BlockInfo entries defined for the block. This should only be used if there are no local abbrevs? (if true add a note?) https://codereview.chromium.org/14813032/diff/42001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:266: std::vector<NaClBitCodeAbbrev*>::size_type NumAbbrevs = nit: Wow, this type is a bit hard to read =) This isn't just a "size_t"? In the above code they had to static_cast<unsigned>. It just needs to be be able to be added to the naclbitc::DEFAULT_MAX_ABBREV right, so perhaps it would be simpler to just cast it, or use size_t if possible? https://codereview.chromium.org/14813032/diff/42001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/42001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1684: // instances: CONSTANTS_BLOCK, FUNCTION_BLOCK and VALUE_SYMTAB_BLOCK. also adding in METADATA_STRING now...
Message was sent while issue was closed.
Committed patchset #8 manually as r80b7ba7 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/14813032/diff/42001/include/llvm/Bitcode/NaCl... File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/14813032/diff/42001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:263: /// (global) BlockInfo entries defined for the block. On 2013/05/21 23:56:45, jvoung (cr) wrote: > This should only be used if there are no local abbrevs? (if true add a note?) Done. https://codereview.chromium.org/14813032/diff/42001/include/llvm/Bitcode/NaCl... include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:266: std::vector<NaClBitCodeAbbrev*>::size_type NumAbbrevs = On 2013/05/21 23:56:45, jvoung (cr) wrote: > nit: Wow, this type is a bit hard to read =) This isn't just a "size_t"? In > the above code they had to static_cast<unsigned>. It just needs to be be able to > be added to the naclbitc::DEFAULT_MAX_ABBREV right, so perhaps it would be > simpler to just cast it, or use size_t if possible? > Simplified to size_t. https://codereview.chromium.org/14813032/diff/42001/lib/Bitcode/NaCl/Writer/N... File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right): https://codereview.chromium.org/14813032/diff/42001/lib/Bitcode/NaCl/Writer/N... lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1684: // instances: CONSTANTS_BLOCK, FUNCTION_BLOCK and VALUE_SYMTAB_BLOCK. On 2013/05/21 23:56:45, jvoung (cr) wrote: > also adding in METADATA_STRING now... Done.
Message was sent while issue was closed.
This change appears to cause a test failure:
******************** TEST 'LLVM :: NaCl/Bitcode/struct-types.ll' FAILED
********************
...
Malformed block record
/home/mseaborn/devel/nacl-git3/native_client/pnacl/git/llvm/test/NaCl/Bitcode/struct-types.ll:53:10:
error: expected string not found in input
; PNACL: <TYPE_BLOCK_ID {{.*}}>
^
<stdin>:1:1: note: scanning from here
<MODULE_BLOCK NumWords=97 BlockCodeSize=1>
^
<stdin>:1:3: note: possible intended match here
<MODULE_BLOCK NumWords=97 BlockCodeSize=1>
On 24 May 2013 12:03, <mseaborn@chromium.org> wrote: > This change appears to cause a test failure: > > ******************** TEST 'LLVM :: NaCl/Bitcode/struct-types.ll' FAILED > ******************** > ... > Malformed block record > /home/mseaborn/devel/nacl-**git3/native_client/pnacl/git/** > llvm/test/NaCl/Bitcode/struct-**types.ll:53:10: > error: expected string not found in input > ; PNACL: <TYPE_BLOCK_ID {{.*}}> > ^ > <stdin>:1:1: note: scanning from here > <MODULE_BLOCK NumWords=97 BlockCodeSize=1> > ^ > <stdin>:1:3: note: possible intended match here > <MODULE_BLOCK NumWords=97 BlockCodeSize=1> > My mistake! I was rebuilding with "ONLY_TOOLS=opt pnacl-abicheck pnacl-freeze" before running the tests. Rebuilding all the targets makes the tests pass. Silly me. Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews?hl=en. For more options, visit https://groups.google.com/groups/opt_out. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
