Make abbreviations explicit in pnacl-freeze/thaw.

include/llvm/Bitcode/NaCl/NaClBitCodes.h

Index: include/llvm/Bitcode/NaCl/NaClBitCodes.h
diff --git a/include/llvm/Bitcode/NaCl/NaClBitCodes.h b/include/llvm/Bitcode/NaCl/NaClBitCodes.h
index 4c0f754f7bab3ef33369679f15bdbe5b0bc829d9..798b37d5c8dad574ba8f88ee6e9d7b68a39def41 100644
--- a/include/llvm/Bitcode/NaCl/NaClBitCodes.h
+++ b/include/llvm/Bitcode/NaCl/NaClBitCodes.h
@@ -17,10 +17,10 @@
 
 #ifndef LLVM_BITCODE_NACL_NACLBITCODES_H
 #define LLVM_BITCODE_NACL_NACLBITCODES_H
-

[jvoung (off chromium), 2013/05/17 23:40:48]

Leave this newline in between the ifndef and the first include.

[Karl, 2013/05/20 22:59:20]

Done.

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/MathExtras.h"
 #include <cassert>
 
 namespace llvm {
@@ -49,7 +49,21 @@ namespace naclbitc {
     UNABBREV_RECORD = 3,
 
     // This is not a code, this is a marker for the first abbrev assignment.
-    FIRST_APPLICATION_ABBREV = 4
+    // In addition, we assume up to two additional enumerated constants are
+    // added for each extension. These constants are:
+    //
+    //   PREFIX_MAX_FIXED_ABBREV
+    //   PREFIX_MAX_ABBREV
+    //
+    // PREFIX_MAX_ABBREV defines the maximal enumeration value used for
+    // the code selector of a block. If Both PREFIX_MAX_FIXED_ABBREV
+    // and PREFIX_MAX_ABBREV is defined, then PREFIX_MAX_FIXED_ABBREV
+    // defines the last code selector of the block that must be read using
+    // a single read (i.e. a FIXED read, or the first chunk of a VBR read.
+    FIRST_APPLICATION_ABBREV = 4,
+    // Defines default values for code length, if no additional selectors
+    // are added.
+    DEFAULT_MAX_ABBREV = FIRST_APPLICATION_ABBREV-1

[jvoung (off chromium), 2013/05/17 23:40:48]

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)

[Karl, 2013/05/20 22:59:20]

Yes, they should have used log32_32_Ceil(3) rather than 3.

[jvoung (off chromium), 2013/05/21 17:24:17]

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?

   };
 
   /// StandardBlockIDs - All bitcode files can optionally include a BLOCKINFO
@@ -184,6 +198,74 @@ public:
     OperandList.push_back(OpInfo);
   }
 };
+
+// NaClBitsNeededForValue - Returns number of bits needed to encode
+// value for dense FIXED encoding.
+inline unsigned NaClBitsNeededForValue(unsigned Value) {
+  // Note: Need to handle case where Value=0xFFFFFFFF as special case,
+  // since we can't add 1 to it.
+  if (Value >= 0x80000000) return 32;
+  return Log2_32_Ceil(Value+1);
+}
+
+/// encodeSignRotatedValue - Encode a signed value by moving the sign

[jvoung (off chromium), 2013/05/17 23:40:48]

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.

[Karl, 2013/05/20 22:59:20]

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

+/// to the LSB for dense VBR encoding.
+inline uint64_t NaClEncodeSignRotatedValue(int64_t V) {
+  return (V >= 0) ? (V << 1) : ((-V << 1) | 1);
+}
+
+/// decodeSignRotatedValue - Decode a signed value stored with the sign bit in
+/// the LSB for dense VBR encoding.
+inline uint64_t NaClDecodeSignRotatedValue(uint64_t V) {
+  if ((V & 1) == 0)
+    return V >> 1;
+  if (V != 1)
+    return -(V >> 1);
+  // There is no such thing as -0 with integers.  "-0" really means MININT.
+  return 1ULL << 63;
+}
+
+// NaClBitcodeSelectorAbbrev - This class determines whether a FIXED or VBR
+// abbreviation should be used for the selector, and the number of bits
+// needed to capture such selectors.
+class NaClBitcodeSelectorAbbrev {
+
+public:
+  // If true, use a FIXED abbreviation. Otherwise, use a VBR abbreviation.
+  bool IsFixed;
+  // Number of bits needed for selector.
+  unsigned NumBits;
+
+  // Creates a selector range for the given values.
+  NaClBitcodeSelectorAbbrev(bool IF, unsigned NB)
+      : IsFixed(IF), NumBits(NB) {}
+
+  // Creates a selector range when no abbreviations are defined.
+  NaClBitcodeSelectorAbbrev()
+      : IsFixed(true),
+        NumBits(NaClBitsNeededForValue(naclbitc::DEFAULT_MAX_ABBREV)) {}
+
+  // Creates a selector range to handle fixed abbrevations up to
+  // the specified value.
+  NaClBitcodeSelectorAbbrev(unsigned MaxAbbrev)

[jvoung (off chromium), 2013/05/17 23:40:48]

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.

[Karl, 2013/05/20 22:59:20]

Made explicit.

+      : IsFixed(true),
+        NumBits(NaClBitsNeededForValue(MaxAbbrev)) {}
+
+  // Creates a selector range where the first chunk can handle all
+  // values < FirstChunkCutoff, and all range values are less than
+  // MaxAbbrev.
+  NaClBitcodeSelectorAbbrev(unsigned FirstChunkCutoff, unsigned MaxAbbrev)
+      : IsFixed(FirstChunkCutoff == MaxAbbrev),
+        NumBits(NaClBitsNeededForValue(FirstChunkCutoff)) {
+    // Before returning, patch up values, based on behaviour of
+    // bitstream writer. Note that VBR requires an extra bit to
+    // define if further bit chunks are needed. Hence, add this information.
+    if (!IsFixed) {
+      ++NumBits;
+      IsFixed = ((1U << NumBits) > MaxAbbrev);
+    }
+  }
+};
 } // End llvm namespace
 
 #endif