Clean up vestigial code for constants in PNaCl bitcode files.

* Removes code that is no longer used because global variables are handled
separately.

* Removes CST_CODE_NULL.
* Adds special abbreviations for constant integer 0, replacing
  most of what was lost when CST_CODE_NULL is removed.
* Adds abbreviation for floating point constants.
* Removes CST_CODE_AGGREGATE and CST_CODE_DATA, except for reading
  old PNaCl version 1 bitcode files.
* Removes call to ParseConstants() in ParseModule() since the module
  block no longer contains a constants block (globals are handled
  separately).
* Removes getConstantFwdRef() method, since it is no longer needed.
* Remove ConstantPlaceHolder class, since it is no longer needed.
* Remove ResolvedConstants and ResolveConstantForwardRefs()
from the bitcode reader, since it is no longer needed.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3668
R=mseaborn@chromium.org

Review URL: https://codereview.chromium.org/23522024

5 files changed, 165 insertions, 331 deletions

diff --git a/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h b/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h
index 2e3cd21652..cf1c98febf 100644
--- a/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h
+++ b/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h
@@ -153,12 +153,18 @@ namespace naclbitc {
   // constant and maintains an implicit current type value.
   enum NaClConstantsCodes {
     CST_CODE_SETTYPE       =  1,  // SETTYPE:       [typeid]
-    CST_CODE_NULL          =  2,  // NULL
+    CST_CODE_NULL          =  2,  // PNaCl version 1 (early versions only).
+                                  //    NULL
+                                  // Not used in PNaCl otherwise (i.e.
+                                  // PNaCl versions 1+).
     CST_CODE_UNDEF         =  3,  // UNDEF
     CST_CODE_INTEGER       =  4,  // INTEGER:       [intval]
     CST_CODE_WIDE_INTEGER  =  5,  // Not used in PNaCl.
     CST_CODE_FLOAT         =  6,  // FLOAT:         [fpval]
-    CST_CODE_AGGREGATE     =  7,  // AGGREGATE:     [n x value number]
+    CST_CODE_AGGREGATE     =  7,  // PNaCl version 1 (early versions only).
+                                  // AGGREGATE:     [n x value number]
+                                  // Not used in PNaCl otherwise (i.e.
+                                  // PNaCl versions 1+).
     CST_CODE_STRING        =  8,  // Not used in PNaCl.
     CST_CODE_CSTRING       =  9,  // Not used in PNaCl.
     CST_CODE_CE_BINOP      = 10,  // Not used in PNaCl.
@@ -173,7 +179,10 @@ namespace naclbitc {
     CST_CODE_CE_SHUFVEC_EX = 19,  // Not used in PNaCl.
     CST_CODE_CE_INBOUNDS_GEP = 20,// Not used in PNaCl.
     CST_CODE_BLOCKADDRESS  = 21,  // Not used in PNaCl.
-    CST_CODE_DATA          = 22,  // DATA:          [n x elements]
+    CST_CODE_DATA          = 22,  // PNaCl version 1 (early versions only).
+                                  // DATA:          [n x elements]
+                                  // Not used in PNaCl otherwise (i.e.
+                                  // PNaCl versions 1+).
     CST_CODE_INLINEASM     = 23   // Not used in PNaCl.
   };
 
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
index 29a842c948..937012feb8 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
@@ -128,42 +128,6 @@ static CallingConv::ID GetDecodedCallingConv(unsigned Val) {
   }
 }
 
-namespace llvm {
-namespace {
-  /// @brief A class for maintaining the slot number definition
-  /// as a placeholder for the actual definition for forward constants defs.
-  class ConstantPlaceHolder : public ConstantExpr {
-    void operator=(const ConstantPlaceHolder &) LLVM_DELETED_FUNCTION;
-  public:
-    // allocate space for exactly one operand
-    void *operator new(size_t s) {
-      return User::operator new(s, 1);
-    }
-    explicit ConstantPlaceHolder(Type *Ty, LLVMContext& Context)
-      : ConstantExpr(Ty, Instruction::UserOp1, &Op<0>(), 1) {
-      Op<0>() = UndefValue::get(Type::getInt32Ty(Context));
-    }
-
-    /// @brief Methods to support type inquiry through isa, cast, and dyn_cast.
-    static bool classof(const Value *V) {
-      return isa<ConstantExpr>(V) &&
-             cast<ConstantExpr>(V)->getOpcode() == Instruction::UserOp1;
-    }
-
-
-    /// Provide fast operand accessors
-    //DECLARE_TRANSPARENT_OPERAND_ACCESSORS(Value);
-  };
-}
-
-// FIXME: can we inherit this from ConstantExpr?
-template <>
-struct OperandTraits<ConstantPlaceHolder> :
-  public FixedNumOperandTraits<ConstantPlaceHolder, 1> {
-};
-}
-
-
 void NaClBitcodeReaderValueList::AssignValue(Value *V, unsigned Idx) {
   assert(V);
   if (Idx == size()) {
@@ -180,17 +144,10 @@ void NaClBitcodeReaderValueList::AssignValue(Value *V, unsigned Idx) {
     return;
   }
 
-  // Handle constants and non-constants (e.g. instrs) differently for
-  // efficiency.
-  if (Constant *PHC = dyn_cast<Constant>(&*OldV)) {
-    ResolveConstants.push_back(std::make_pair(PHC, Idx));
-    OldV = V;
-  } else {
-    // If there was a forward reference to this value, replace it.
-    Value *PrevVal = OldV;
-    OldV->replaceAllUsesWith(V);
-    delete PrevVal;
-  }
+  // If there was a forward reference to this value, replace it.
+  Value *PrevVal = OldV;
+  OldV->replaceAllUsesWith(V);
+  delete PrevVal;
 }
 
 void NaClBitcodeReaderValueList::AssignGlobalVar(GlobalVariable *GV,
@@ -220,22 +177,6 @@ void NaClBitcodeReaderValueList::AssignGlobalVar(GlobalVariable *GV,
   ValuePtrs[Idx] = GV;
 }
 
-Constant *NaClBitcodeReaderValueList::getConstantFwdRef(unsigned Idx,
-                                                        Type *Ty) {
-  if (Idx >= size())
-    resize(Idx + 1);
-
-  if (Value *V = ValuePtrs[Idx]) {
-    assert(Ty == V->getType() && "Type mismatch in constant table!");
-    return cast<Constant>(V);
-  }
-
-  // Create and return a placeholder, which will later be RAUW'd.
-  Constant *C = new ConstantPlaceHolder(Ty, Context);
-  ValuePtrs[Idx] = C;
-  return C;
-}
-
 Value *NaClBitcodeReaderValueList::getValueFwdRef(unsigned Idx) {
   if (Idx >= size())
     return 0;
@@ -283,88 +224,6 @@ Constant *NaClBitcodeReaderValueList::getOrCreateGlobalVarRef(
   return C;
 }
 
-/// ResolveConstantForwardRefs - Once all constants are read, this method bulk
-/// resolves any forward references.  The idea behind this is that we sometimes
-/// get constants (such as large arrays) which reference *many* forward ref
-/// constants.  Replacing each of these causes a lot of thrashing when
-/// building/reuniquing the constant.  Instead of doing this, we look at all the
-/// uses and rewrite all the place holders at once for any constant that uses
-/// a placeholder.
-void NaClBitcodeReaderValueList::ResolveConstantForwardRefs() {
-  // Sort the values by-pointer so that they are efficient to look up with a
-  // binary search.
-  std::sort(ResolveConstants.begin(), ResolveConstants.end());
-
-  SmallVector<Constant*, 64> NewOps;
-
-  while (!ResolveConstants.empty()) {
-    Value *RealVal = operator[](ResolveConstants.back().second);
-    Constant *Placeholder = ResolveConstants.back().first;
-    ResolveConstants.pop_back();
-
-    // Loop over all users of the placeholder, updating them to reference the
-    // new value.  If they reference more than one placeholder, update them all
-    // at once.
-    while (!Placeholder->use_empty()) {
-      Value::use_iterator UI = Placeholder->use_begin();
-      User *U = *UI;
-
-      // If the using object isn't uniqued, just update the operands.  This
-      // handles instructions and initializers for global variables.
-      if (!isa<Constant>(U) || isa<GlobalValue>(U)) {
-        UI.getUse().set(RealVal);
-        continue;
-      }
-
-      // Otherwise, we have a constant that uses the placeholder.  Replace that
-      // constant with a new constant that has *all* placeholder uses updated.
-      Constant *UserC = cast<Constant>(U);
-      for (User::op_iterator I = UserC->op_begin(), E = UserC->op_end();
-           I != E; ++I) {
-        Value *NewOp;
-        if (!isa<ConstantPlaceHolder>(*I)) {
-          // Not a placeholder reference.
-          NewOp = *I;
-        } else if (*I == Placeholder) {
-          // Common case is that it just references this one placeholder.
-          NewOp = RealVal;
-        } else {
-          // Otherwise, look up the placeholder in ResolveConstants.
-          ResolveConstantsTy::iterator It =
-            std::lower_bound(ResolveConstants.begin(), ResolveConstants.end(),
-                             std::pair<Constant*, unsigned>(cast<Constant>(*I),
-                                                            0));
-          assert(It != ResolveConstants.end() && It->first == *I);
-          NewOp = operator[](It->second);
-        }
-
-        NewOps.push_back(cast<Constant>(NewOp));
-      }
-
-      // Make the new constant.
-      Constant *NewC;
-      if (ConstantArray *UserCA = dyn_cast<ConstantArray>(UserC)) {
-        NewC = ConstantArray::get(UserCA->getType(), NewOps);
-      } else if (ConstantStruct *UserCS = dyn_cast<ConstantStruct>(UserC)) {
-        NewC = ConstantStruct::get(UserCS->getType(), NewOps);
-      } else if (isa<ConstantVector>(UserC)) {
-        NewC = ConstantVector::get(NewOps);
-      } else {
-        assert(isa<ConstantExpr>(UserC) && "Must be a ConstantExpr.");
-        NewC = cast<ConstantExpr>(UserC)->getWithOperands(NewOps);
-      }
-
-      UserC->replaceAllUsesWith(NewC);
-      UserC->destroyConstant();
-      NewOps.clear();
-    }
-
-    // Update all ValueHandles, they should be the only users at this point.
-    Placeholder->replaceAllUsesWith(RealVal);
-    delete Placeholder;
-  }
-}
-
 Type *NaClBitcodeReader::getTypeByID(unsigned ID) {
   // The type table size is always specified correctly.
   if (ID >= TypeList.size())
@@ -770,10 +629,6 @@ bool NaClBitcodeReader::ParseConstants() {
     case NaClBitstreamEntry::EndBlock:
       if (NextCstNo != ValueList.size())
         return Error("Invalid constant reference!");
-
-      // Once all the constants have been read, go through and resolve forward
-      // references.
-      ValueList.ResolveConstantForwardRefs();
       DEBUG(dbgs() << "<- ParseConstants\n");
       return false;
     case NaClBitstreamEntry::Record:
@@ -804,6 +659,10 @@ bool NaClBitcodeReader::ParseConstants() {
       CurTy = TypeList[Record[0]];
       continue;  // Skip the ValueList manipulation.
     case naclbitc::CST_CODE_NULL:      // NULL
+      // Deprecated. Only exists in early PNaCl version 1 bitcode files.
+      // TODO(kschimpf) Remove this as soon as is feasible.
+      if (GetPNaClVersion() >= 2)
+        return Error("null constants not supported in PNaCl bitcode");
       V = Constant::getNullValue(CurTy);
       break;
     case naclbitc::CST_CODE_INTEGER:   // INTEGER: [intval]
@@ -826,81 +685,43 @@ bool NaClBitcodeReader::ParseConstants() {
     }
 
     case naclbitc::CST_CODE_AGGREGATE: {// AGGREGATE: [n x value number]
+      if (GetPNaClVersion() >= 2)
+        return Error("Aggregate constants not supported in PNaCl bitcode");
       if (Record.empty())
         return Error("Invalid CST_AGGREGATE record");
 
-      unsigned Size = Record.size();
-      SmallVector<Constant*, 16> Elts;
-
-      if (StructType *STy = dyn_cast<StructType>(CurTy)) {
-        for (unsigned i = 0; i != Size; ++i)
-          Elts.push_back(ValueList.getConstantFwdRef(Record[i],
-                                                     STy->getElementType(i)));
-        V = ConstantStruct::get(STy, Elts);
-      } else if (ArrayType *ATy = dyn_cast<ArrayType>(CurTy)) {
-        Type *EltTy = ATy->getElementType();
-        for (unsigned i = 0; i != Size; ++i)
-          Elts.push_back(ValueList.getConstantFwdRef(Record[i], EltTy));
-        V = ConstantArray::get(ATy, Elts);
-      } else if (VectorType *VTy = dyn_cast<VectorType>(CurTy)) {
-        Type *EltTy = VTy->getElementType();
-        for (unsigned i = 0; i != Size; ++i)
-          Elts.push_back(ValueList.getConstantFwdRef(Record[i], EltTy));
-        V = ConstantVector::get(Elts);
-      } else {
-        V = UndefValue::get(CurTy);
-      }
+      // Note: The only structured types in early PNaCl version 1 bitcode files
+      // are those used to define selection lists in the switch instruction.
+      // However, these selection lists are not encoded as part of the switch
+      // instruction. Hence, the corresponding aggregate constants are not
+      // used. Hence, using undefined is sufficient.
+      //
+      // This has been fixed in PNaCl version 1+. We only need to handle
+      // old PNaCl version 1 bitcode files.
+      //
+      // TODO(kschimpf): Remove this as soon as feasible.
+
+      V = UndefValue::get(CurTy);
       break;
     }
     case naclbitc::CST_CODE_DATA: {// DATA: [n x value]
+      if (GetPNaClVersion() >= 2)
+        return Error("Data constants not supported in PNaCl bitcode");
       if (Record.empty())
         return Error("Invalid CST_DATA record");
 
-      Type *EltTy = cast<SequentialType>(CurTy)->getElementType();
-      unsigned Size = Record.size();
+      // Note: The only structured types in early PNaCl version 1 bitcode files
+      // are those used to define selection lists in switch instruction
+      // However, these selection lists are not encoded as part of the switch
+      // instruction. Hence, the corresponding data constants are not
+      // used. Hence, using undefined is sufficient.
+      //
+      // This has been fixed in PNaCl version 1+. We only need to handle
+      // old PNaCl version 1 bitcode files.
+      //
+      // TODO(kschimpf): Remove this as soon as feasible.
 
-      if (EltTy->isIntegerTy(8)) {
-        SmallVector<uint8_t, 16> Elts(Record.begin(), Record.end());
-        if (isa<VectorType>(CurTy))
-          V = ConstantDataVector::get(Context, Elts);
-        else
-          V = ConstantDataArray::get(Context, Elts);
-      } else if (EltTy->isIntegerTy(16)) {
-        SmallVector<uint16_t, 16> Elts(Record.begin(), Record.end());
-        if (isa<VectorType>(CurTy))
-          V = ConstantDataVector::get(Context, Elts);
-        else
-          V = ConstantDataArray::get(Context, Elts);
-      } else if (EltTy->isIntegerTy(32)) {
-        SmallVector<uint32_t, 16> Elts(Record.begin(), Record.end());
-        if (isa<VectorType>(CurTy))
-          V = ConstantDataVector::get(Context, Elts);
-        else
-          V = ConstantDataArray::get(Context, Elts);
-      } else if (EltTy->isIntegerTy(64)) {
-        SmallVector<uint64_t, 16> Elts(Record.begin(), Record.end());
-        if (isa<VectorType>(CurTy))
-          V = ConstantDataVector::get(Context, Elts);
-        else
-          V = ConstantDataArray::get(Context, Elts);
-      } else if (EltTy->isFloatTy()) {
-        SmallVector<float, 16> Elts(Size);
-        std::transform(Record.begin(), Record.end(), Elts.begin(), BitsToFloat);
-        if (isa<VectorType>(CurTy))
-          V = ConstantDataVector::get(Context, Elts);
-        else
-          V = ConstantDataArray::get(Context, Elts);
-      } else if (EltTy->isDoubleTy()) {
-        SmallVector<double, 16> Elts(Size);
-        std::transform(Record.begin(), Record.end(), Elts.begin(),
-                       BitsToDouble);
-        if (isa<VectorType>(CurTy))
-          V = ConstantDataVector::get(Context, Elts);
-        else
-          V = ConstantDataArray::get(Context, Elts);
-      } else {
-        return Error("Unknown element type in CE_DATA");
-      }
+      V = UndefValue::get(CurTy);
       break;
     }
     }
@@ -973,11 +794,12 @@ bool NaClBitcodeReader::ParseModule(bool Resume) {
 
     case NaClBitstreamEntry::SubBlock:
       switch (Entry.ID) {
-      default:  // Skip unknown content.
-        DEBUG(dbgs() << "Skip unknown context\n");
-        if (Stream.SkipBlock())
-          return Error("Malformed block record");
-        break;
+      default: {
+        std::string Message;
+        raw_string_ostream StrM(Message);
+        StrM << "Unknown block ID: " << Entry.ID;
+        return Error(StrM.str());
+      }
       case naclbitc::BLOCKINFO_BLOCK_ID:
         if (Stream.ReadBlockInfoBlock())
           return Error("Malformed BlockInfoBlock");
@@ -995,10 +817,6 @@ bool NaClBitcodeReader::ParseModule(bool Resume) {
           return true;
         SeenValueSymbolTable = true;
         break;
-      case naclbitc::CONSTANTS_BLOCK_ID:
-        if (ParseConstants())
-          return true;
-        break;
       case naclbitc::FUNCTION_BLOCK_ID:
         // If this is the first function body we've seen, reverse the
         // FunctionsWithBodies list.
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
index f69fdbdfd4..318fcb1470 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
@@ -77,22 +77,10 @@ public:
 
 class NaClBitcodeReaderValueList {
   std::vector<WeakVH> ValuePtrs;
-
-  /// ResolveConstants - As we resolve forward-referenced constants, we add
-  /// information about them to this vector.  This allows us to resolve them in
-  /// bulk instead of resolving each reference at a time.  See the code in
-  /// ResolveConstantForwardRefs for more information about this.
-  ///
-  /// The key of this vector is the placeholder constant, the value is the slot
-  /// number that holds the resolved value.
-  typedef std::vector<std::pair<Constant*, unsigned> > ResolveConstantsTy;
-  ResolveConstantsTy ResolveConstants;
   LLVMContext &Context;
 public:
   NaClBitcodeReaderValueList(LLVMContext &C) : Context(C) {}
-  ~NaClBitcodeReaderValueList() {
-    assert(ResolveConstants.empty() && "Constants not resolved?");
-  }
+  ~NaClBitcodeReaderValueList() {}
 
   // vector compatibility methods
   unsigned size() const { return ValuePtrs.size(); }
@@ -102,7 +90,6 @@ public:
   }
 
   void clear() {
-    assert(ResolveConstants.empty() && "Constants not resolved?");
     ValuePtrs.clear();
   }
 
@@ -124,12 +111,6 @@ public:
   // already been declared.
   bool createValueFwdRef(unsigned Idx, Type *Ty);
 
-  // Declares the type of the forward-referenced constant Idx.
-  // Returns 0 if an error occurred.
-  // TODO(kschimpf) Convert these to be like createValueFwdRef and
-  // getValueFwdRef.
-  Constant *getConstantFwdRef(unsigned Idx, Type *Ty);
-
   // Gets the forward reference value for Idx.
   Value *getValueFwdRef(unsigned Idx);
 
@@ -149,10 +130,6 @@ public:
   // replaces uses of the global variable forward reference with the
   // value GV.
   void AssignGlobalVar(GlobalVariable *GV, unsigned Idx);
-
-  /// ResolveConstantForwardRefs - Once all constants are read, this method bulk
-  /// resolves any forward references.
-  void ResolveConstantForwardRefs();
 };
 
 
diff --git a/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp b/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
index 4288aceb06..3355bc757c 100644
--- a/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
+++ b/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
@@ -63,12 +63,9 @@ enum {
   // CONSTANTS_BLOCK abbrev id's.
   CONSTANTS_SETTYPE_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
   CONSTANTS_INTEGER_ABBREV,
-  CONSTANTS_NULL_Abbrev,
-  CONSTANTS_MAX_ABBREV = CONSTANTS_NULL_Abbrev,
-
-  // CONSTANTS_BLOCK abbrev id's when global (extends list above).
-  CST_CONSTANTS_AGGREGATE_ABBREV = CONSTANTS_MAX_ABBREV+1,
-  CST_CONSTANTS_MAX_ABBREV = CST_CONSTANTS_AGGREGATE_ABBREV,
+  CONSTANTS_INTEGER_ZERO_ABBREV,
+  CONSTANTS_FLOAT_ABBREV,
+  CONSTANTS_MAX_ABBREV = CONSTANTS_FLOAT_ABBREV,
 
   // GLOBALVAR BLOCK abbrev id's.
   GLOBALVAR_VAR_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
@@ -476,7 +473,8 @@ static void EmitAPInt(SmallVectorImpl<uint64_t> &Vals,
     uint64_t V = Val.getSExtValue();
     emitSignedInt64(Vals, V);
     Code = naclbitc::CST_CODE_INTEGER;
-    AbbrevToUse = CONSTANTS_INTEGER_ABBREV;
+    AbbrevToUse =
+        Val == 0 ? CONSTANTS_INTEGER_ZERO_ABBREV : CONSTANTS_INTEGER_ABBREV;
   } else {
     report_fatal_error("Wide integers are not supported");
   }
@@ -484,33 +482,12 @@ static void EmitAPInt(SmallVectorImpl<uint64_t> &Vals,
 
 static void WriteConstants(unsigned FirstVal, unsigned LastVal,
                            const NaClValueEnumerator &VE,
-                           NaClBitstreamWriter &Stream, bool isGlobal) {
+                           NaClBitstreamWriter &Stream) {
   if (FirstVal == LastVal) return;
 
-  Stream.EnterSubblock(naclbitc::CONSTANTS_BLOCK_ID,
-                       (isGlobal
-                        ? CST_CONSTANTS_MAX_ABBREV
-                        : CONSTANTS_MAX_ABBREV));
+  Stream.EnterSubblock(naclbitc::CONSTANTS_BLOCK_ID, CONSTANTS_MAX_ABBREV);
 
   unsigned AggregateAbbrev = 0;
-  // If this is a constant pool for the module, emit module-specific abbrevs.
-  // Note: These abbreviations are size specific (to LastVal), and hence,
-  // can be more efficient if LastVal is known (rather then generating
-  // up-front for all constant sections).
-  if (isGlobal) {
-    // Abbrev for CST_CODE_AGGREGATE.
-    NaClBitCodeAbbrev *Abbv = new NaClBitCodeAbbrev();
-    Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::CST_CODE_AGGREGATE));
-    Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Array));
-    Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed,
-                                  NaClBitsNeededForValue(LastVal)));
-    AggregateAbbrev = Stream.EmitAbbrev(Abbv);
-    if (CST_CONSTANTS_AGGREGATE_ABBREV != AggregateAbbrev)
-      llvm_unreachable("Unexpected abbrev ordering!");
-
-    DEBUG(dbgs() << "-- emitted abbreviations\n");
-  }
-
 
   SmallVector<uint64_t, 64> Record;
 
@@ -533,47 +510,19 @@ static void WriteConstants(unsigned FirstVal, unsigned LastVal,
     const Constant *C = cast<Constant>(V);
     unsigned Code = -1U;
     unsigned AbbrevToUse = 0;
-    if (C->isNullValue()) {
-      Code = naclbitc::CST_CODE_NULL;
-    } else if (isa<UndefValue>(C)) {
+    if (isa<UndefValue>(C)) {
       Code = naclbitc::CST_CODE_UNDEF;
     } else if (const ConstantInt *IV = dyn_cast<ConstantInt>(C)) {
       EmitAPInt(Record, Code, AbbrevToUse, IV->getValue());
     } else if (const ConstantFP *CFP = dyn_cast<ConstantFP>(C)) {
       Code = naclbitc::CST_CODE_FLOAT;
+      AbbrevToUse = CONSTANTS_FLOAT_ABBREV;
       Type *Ty = CFP->getType();
       if (Ty->isFloatTy() || Ty->isDoubleTy()) {
         Record.push_back(CFP->getValueAPF().bitcastToAPInt().getZExtValue());
       } else {
         report_fatal_error("Unknown FP type");
       }
-    } else if (const ConstantDataSequential *CDS =
-                  dyn_cast<ConstantDataSequential>(C)) {
-      Code = naclbitc::CST_CODE_DATA;
-      Type *EltTy = CDS->getType()->getElementType();
-      if (isa<IntegerType>(EltTy)) {
-        for (unsigned i = 0, e = CDS->getNumElements(); i != e; ++i)
-          Record.push_back(CDS->getElementAsInteger(i));
-      } else if (EltTy->isFloatTy()) {
-        for (unsigned i = 0, e = CDS->getNumElements(); i != e; ++i) {
-          union { float F; uint32_t I; };
-          F = CDS->getElementAsFloat(i);
-          Record.push_back(I);
-        }
-      } else {
-        assert(EltTy->isDoubleTy() && "Unknown ConstantData element type");
-        for (unsigned i = 0, e = CDS->getNumElements(); i != e; ++i) {
-          union { double F; uint64_t I; };
-          F = CDS->getElementAsDouble(i);
-          Record.push_back(I);
-        }
-      }
-    } else if (isa<ConstantArray>(C) || isa<ConstantStruct>(C) ||
-               isa<ConstantVector>(C)) {
-      Code = naclbitc::CST_CODE_AGGREGATE;
-      for (unsigned i = 0, e = C->getNumOperands(); i != e; ++i)
-        Record.push_back(VE.getValueID(C->getOperand(i)));
-      AbbrevToUse = AggregateAbbrev;
     } else {
 #ifndef NDEBUG
       C->dump();
@@ -588,20 +537,6 @@ static void WriteConstants(unsigned FirstVal, unsigned LastVal,
   DEBUG(dbgs() << "<- WriteConstants\n");
 }
 
-static void WriteModuleConstants(const NaClValueEnumerator &VE,
-                                 NaClBitstreamWriter &Stream) {
-  const NaClValueEnumerator::ValueList &Vals = VE.getValues();
-
-  // Find the first constant to emit, which is the first non-globalvalue value.
-  // We know globalvalues have been emitted by WriteModuleInfo.
-  for (unsigned i = 0, e = Vals.size(); i != e; ++i) {
-    if (!isa<GlobalValue>(Vals[i].first)) {
-      WriteConstants(i, Vals.size(), VE, Stream, true);
-      return;
-    }
-  }
-}
-
 /// \brief Emits a type for the forward value reference. That is, if
 /// the ID for the given value is larger than or equal to the BaseID,
 /// the corresponding forward reference is generated.
@@ -984,7 +919,7 @@ static void WriteFunction(const Function &F, NaClValueEnumerator &VE,
   // If there are function-local constants, emit them now.
   unsigned CstStart, CstEnd;
   VE.getFunctionConstantRange(CstStart, CstEnd);
-  WriteConstants(CstStart, CstEnd, VE, Stream, false);
+  WriteConstants(CstStart, CstEnd, VE, Stream);
 
   // Keep a running idea of what the instruction ID is.
   unsigned InstID = CstEnd;
@@ -1076,11 +1011,20 @@ static void WriteBlockInfo(const NaClValueEnumerator &VE,
                                    Abbv) != CONSTANTS_INTEGER_ABBREV)
       llvm_unreachable("Unexpected abbrev ordering!");
   }
-  { // NULL abbrev for CONSTANTS_BLOCK.
+  { // INTEGER_ZERO abbrev for CONSTANTS_BLOCK.
     NaClBitCodeAbbrev *Abbv = new NaClBitCodeAbbrev();
-    Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::CST_CODE_NULL));
+    Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::CST_CODE_INTEGER));
+    Abbv->Add(NaClBitCodeAbbrevOp(0));
     if (Stream.EmitBlockInfoAbbrev(naclbitc::CONSTANTS_BLOCK_ID,
-                                   Abbv) != CONSTANTS_NULL_Abbrev)
+                                   Abbv) != CONSTANTS_INTEGER_ZERO_ABBREV)
+      llvm_unreachable("Unexpected abbrev ordering!");
+  }
+  { // FLOAT abbrev for CONSTANTS_BLOCK.
+    NaClBitCodeAbbrev *Abbv = new NaClBitCodeAbbrev();
+    Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::CST_CODE_FLOAT));
+    Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 8));
+    if (Stream.EmitBlockInfoAbbrev(naclbitc::CONSTANTS_BLOCK_ID,
+                                   Abbv) != CONSTANTS_FLOAT_ABBREV)
       llvm_unreachable("Unexpected abbrev ordering!");
   }
 
@@ -1266,9 +1210,6 @@ static void WriteModule(const Module *M, NaClBitstreamWriter &Stream) {
   // descriptors for global variables, and function prototype info.
   WriteModuleInfo(M, VE, Stream);
 
-  // Emit constants.
-  WriteModuleConstants(VE, Stream);
-
   // Emit names for globals/functions etc.
   WriteValueSymbolTable(M->getValueSymbolTable(), VE, Stream);
 
diff --git a/test/NaCl/Bitcode/null.ll b/test/NaCl/Bitcode/null.ll
new file mode 100644
index 0000000000..9e5de493b3
--- /dev/null
+++ b/test/NaCl/Bitcode/null.ll
@@ -0,0 +1,89 @@
+; Test that we no longer generate NULL for numeric constants.
+
+; RUN: llvm-as < %s | pnacl-freeze --pnacl-version=1 \
+; RUN:              | pnacl-bcanalyzer -dump-records \
+; RUN:              | FileCheck %s -check-prefix=PF
+
+; RUN: llvm-as < %s | pnacl-freeze --pnacl-version=1 | pnacl-thaw \
+; RUN:              | llvm-dis - | FileCheck %s -check-prefix=TD
+
+; RUN: llvm-as < %s | pnacl-freeze --pnacl-version=2 \
+; RUN:              | pnacl-bcanalyzer -dump-records \
+; RUN:              | FileCheck %s -check-prefix=PF
+
+; RUN: llvm-as < %s | pnacl-freeze --pnacl-version=2 | pnacl-thaw \
+; RUN:              | llvm-dis - | FileCheck %s -check-prefix=TD
+
+; ------------------------------------------------------
+
+define void @TestIntegers() {
+  %1 = and i1 true, false
+  %2 = add i8 1, 0
+  %3 = add i16 1, 0
+  %4 = add i32 1, 0
+  %5 = add i64 1, 0
+  ret void
+}
+
+; TD:      define void @TestIntegers() {
+; TD-NEXT:   %1 = and i1 true, false
+; TD-NEXT:   %2 = add i8 1, 0
+; TD-NEXT:   %3 = add i16 1, 0
+; TD-NEXT:   %4 = add i32 1, 0
+; TD-NEXT:   %5 = add i64 1, 0
+; TD-NEXT:   ret void
+; TD-NEXT: }
+
+; PF:      <FUNCTION_BLOCK>
+; PF-NEXT:   <DECLAREBLOCKS op0=1/>
+; PF-NEXT:   <CONSTANTS_BLOCK>
+; PF-NEXT:     <SETTYPE op0=1/>
+; PF-NEXT:     <INTEGER op0=3/>
+; PF-NEXT:     <INTEGER op0=0/>
+; PF-NEXT:     <SETTYPE op0=2/>
+; PF-NEXT:     <INTEGER op0=2/>
+; PF-NEXT:     <INTEGER op0=0/>
+; PF-NEXT:     <SETTYPE op0=3/>
+; PF-NEXT:     <INTEGER op0=2/>
+; PF-NEXT:     <INTEGER op0=0/>
+; PF-NEXT:     <SETTYPE op0=4/>
+; PF-NEXT:     <INTEGER op0=2/>
+; PF-NEXT:     <INTEGER op0=0/>
+; PF-NEXT:     <SETTYPE op0=5/>
+; PF-NEXT:     <INTEGER op0=2/>
+; PF-NEXT:     <INTEGER op0=0/>
+; PF-NEXT:   </CONSTANTS_BLOCK>
+; PF-NEXT:   <INST_BINOP op0=10 op1=9 op2=10/>
+; PF-NEXT:   <INST_BINOP op0=9 op1=8 op2=0/>
+; PF-NEXT:   <INST_BINOP op0=8 op1=7 op2=0/>
+; PF-NEXT:   <INST_BINOP op0=7 op1=6 op2=0/>
+; PF-NEXT:   <INST_BINOP op0=6 op1=5 op2=0/>
+; PF-NEXT:   <INST_RET/>
+; PF-NEXT: </FUNCTION_BLOCK>
+
+define void @TestFloats() {
+  %1 = fadd float 1.0, 0.0
+  %2 = fadd double 1.0, 0.0
+  ret void
+}
+
+; TD:      define void @TestFloats() {
+; TD-NEXT:   %1 = fadd float 1.000000e+00, 0.000000e+00
+; TD-NEXT:   %2 = fadd double 1.000000e+00, 0.000000e+00
+; TD-NEXT:   ret void
+; TD-NEXT: }
+
+; PF:      <FUNCTION_BLOCK>
+; PF-NEXT:   <DECLAREBLOCKS op0=1/>
+; PF-NEXT:   <CONSTANTS_BLOCK>
+; PF-NEXT:     <SETTYPE op0=6/>
+; PF-NEXT:     <FLOAT op0=1065353216/>
+; PF-NEXT:     <FLOAT op0=0/>
+; PF-NEXT:     <SETTYPE op0=7/>
+; PF-NEXT:     <FLOAT op0=4607182418800017408/>
+; PF-NEXT:     <FLOAT op0=0/>
+; PF-NEXT:   </CONSTANTS_BLOCK>
+; PF-NEXT:   <INST_BINOP op0=4 op1=3 op2=0/>
+; PF-NEXT:   <INST_BINOP op0=3 op1=2 op2=0/>
+; PF-NEXT:   <INST_RET/>
+; PF-NEXT: </FUNCTION_BLOCK>