Convert Subzero's bitcode reader to generate ICE types.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index af78e2f3e802e3f29af026d6a9f3aa69d2762c20..2c5bb6530ba939407dfdf787cc3380f1640e5ef9 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -49,6 +49,74 @@ static cl::opt<bool> AllowErrorRecovery(
     cl::desc("Allow error recovery when reading PNaCl bitcode."),
     cl::init(false));
 
+// Models elements in the list of types defined in the types block.
+// These elements can be a (simple) type, or a function type
+// signature.
+class ExtendedType {
+  ExtendedType(const ExtendedType &) = delete;
+  ExtendedType &operator=(const ExtendedType &) = delete;
+
+public:
+  // Discriminator for LLVM-style RTTI.
+  enum ExtendedTypeKind { SimpleExtendedKind, FcnSigExtendedKind };
+  virtual ~ExtendedType() {}
+  ExtendedTypeKind getKind() const { return Kind; }
+  virtual void Print(Ice::Ostream &Stream) const = 0;
+
+protected:
+  explicit ExtendedType(ExtendedTypeKind Kind) : Kind(Kind) {}
+
+private:
+  ExtendedTypeKind Kind;
+};
+
+Ice::Ostream &operator<<(Ice::Ostream &Stream, const ExtendedType &Ty) {
+  Ty.Print(Stream);
+  return Stream;
+}
+
+// Models an ICE type as an extended type.
+class SimpleExtendedType : public ExtendedType {
+  SimpleExtendedType(const SimpleExtendedType &) = delete;
+  SimpleExtendedType &operator=(const SimpleExtendedType &) = delete;
+
+public:
+  explicit SimpleExtendedType(Ice::Type SimpleType)
+      : ExtendedType(SimpleExtendedKind), SimpleType(SimpleType) {}
+
+  ~SimpleExtendedType() final {}
+
+  Ice::Type getType() const { return SimpleType; }
+
+  void Print(Ice::Ostream &Stream) const final { Stream << SimpleType; }
+
+  static bool classof(const ExtendedType *Ty) {
+    return Ty->getKind() == SimpleExtendedKind;
+  }
+
+private:
+  Ice::Type SimpleType;
+};
+
+// Models a function signature as an extended type.
+class FcnSigExtendedType : public ExtendedType {
+  FcnSigExtendedType(const FcnSigExtendedType &) = delete;
+  FcnSigExtendedType &operator=(const FcnSigExtendedType &) = delete;
+
+public:
+  explicit FcnSigExtendedType(Ice::Type ReturnType)
+      : ExtendedType(FcnSigExtendedKind), FcnSig(ReturnType) {}
+  const Ice::FcnSigType &getFcnSig() const { return FcnSig; }
+  void appendArgType(Ice::Type ArgType) { FcnSig.appendArgType(ArgType); }
+  void Print(Ice::Ostream &Stream) const final { Stream << FcnSig; }
+  static bool classof(const ExtendedType *Ty) {
+    return Ty->getKind() == FcnSigExtendedKind;
+  }
+
+private:
+  Ice::FcnSigType FcnSig;
+};
+
 // Top-level class to read PNaCl bitcode files, and translate to ICE.
 class TopLevelParser : public NaClBitcodeParser {
   TopLevelParser(const TopLevelParser &) = delete;
@@ -63,12 +131,13 @@ public:
         Header(Header), TypeConverter(getLLVMContext()),
         ErrorStatus(ErrorStatus), NumErrors(0), NumFunctionIds(0),
         NumFunctionBlocks(0),
-        GlobalVarPlaceHolderType(convertToLLVMType(Ice::IceType_i8)) {
+        GlobalVarPlaceHolderType(convertToLLVMType(Ice::IceType_i8)),
+        UndefinedFcnSigType(Ice::IceType_void) {
     Mod->setDataLayout(PNaClDataLayout);
     setErrStream(Translator.getContext()->getStrDump());
   }
 
-  ~TopLevelParser() override {}
+  ~TopLevelParser() override { DeleteContainerPointers(TypeIDValues); }

[Jim Stichnoth, 2014/10/05 15:35:47]

I think that if possible, the memory allocation strategy here should follow the
arena-based style that Cfg uses for Inst allocation.  This is as long as the
objects being allocated can be made POD, particularly with trivial destructors
which also means no container fields.  (The Inst class deals with this by using
flat arrays and specifying the array size in the constructor.)

If this isn't practical now, at least leave a TODO.

[Karl, 2014/10/06 21:15:54]

Reorganized to no longer allocate extended types. Rather, there is a single
representation (that handles both simple and function signature extended types),
and cast is only used to add accessors. This should solve this problem.

 
   Ice::Translator &getTranslator() { return Translator; }
 
@@ -101,18 +170,40 @@ public:
   void resizeTypeIDValues(unsigned NewSize) { TypeIDValues.resize(NewSize); }
 
   /// Returns the type associated with the given index.
-  Type *getTypeByID(unsigned ID) {
+  Ice::Type getTypeByID(unsigned ID) {

[Jim Stichnoth, 2014/10/05 15:35:47]

Can this be const?

I guess not because of setting errors?

I think that these get() methods should either be const, or they should document
why they are not const.

[jvoung (off chromium), 2014/10/06 14:32:17]

I wonder if this should be called "getSimpleTypeByID" to be more specific.

Otherwise it looks close to setTypeID, but that handles both types.

[Karl, 2014/10/06 21:15:54]

Done. Added constness and added qualifier to accessor names.

+    // Note: method resizeTypeIDValues expands TypeIDValues
+    // to the specified size, and fills elements with nullptr.
+    ExtendedType *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : nullptr;
+    if (Ty == nullptr) {
+      reportTypeIDAsUndefined(ID);
+    } else if (auto SimpleTy = dyn_cast<SimpleExtendedType>(Ty)) {
+      return SimpleTy->getType();
+    } else {
+      reportTypeIDNotSimple(ID, Ty);
+    }
+    // TODO(kschimpf) Remove error recovery once implementation complete.
+    return Ice::IceType_void;
+  }
+
+  /// Returns the type signature associated with the given index.
+  const Ice::FcnSigType &getFcnSigTypeByID(unsigned ID) {
     // Note: method resizeTypeIDValues expands TypeIDValues
-    // to the specified size, and fills elements with NULL.
-    Type *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : NULL;
-    if (Ty)
-      return Ty;
-    return reportTypeIDAsUndefined(ID);
+    // to the specified size, and fills elements with nullptr.
+    ExtendedType *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : nullptr;
+    if (Ty == nullptr) {
+      reportFcnSigTypeIDAsUndefined(ID);
+    } else if (auto SigTy = dyn_cast<FcnSigExtendedType>(Ty)) {
+      return SigTy->getFcnSig();
+    } else {
+      reportFcnSigTypeIDNotFcnSig(ID, Ty);
+    }
+    // TODO(kschimpf) Remove error recovery once implementation complete.
+    return UndefinedFcnSigType;
   }
 
   /// Defines type for ID.
-  void setTypeID(unsigned ID, Type *Ty) {
-    if (ID < TypeIDValues.size() && TypeIDValues[ID] == NULL) {
+  void setTypeID(unsigned ID, ExtendedType *Ty) {
+    if (ID < TypeIDValues.size() && TypeIDValues[ID] == nullptr) {
       TypeIDValues[ID] = Ty;
       return;
     }
@@ -144,7 +235,7 @@ public:
   /// Returns the LLVM IR value associatd with the global value ID.
   Value *getGlobalValueByID(unsigned ID) const {
     if (ID >= ValueIDValues.size())
-      return NULL;
+      return nullptr;
     return ValueIDValues[ID];
   }
 
@@ -154,11 +245,11 @@ public:
     // TODO(kschimpf): Can this be built when creating global initializers?
     if (ID >= ValueIDConstants.size()) {
       if (ID >= ValueIDValues.size())
-        return NULL;
+        return nullptr;
       ValueIDConstants.resize(ValueIDValues.size());
     }
     Ice::Constant *C = ValueIDConstants[ID];
-    if (C != NULL)
+    if (C != nullptr)
       return C;
     Value *V = ValueIDValues[ID];
     assert(isa<GlobalValue>(V));
@@ -188,7 +279,7 @@ public:
   /// later.
   Constant *getOrCreateGlobalVarRef(unsigned ID) {
     if (ID >= ValueIDValues.size())
-      return NULL;
+      return nullptr;
     if (Value *C = ValueIDValues[ID])
       return dyn_cast<Constant>(C);
     Constant *C = new GlobalVariable(*Mod, GlobalVarPlaceHolderType, false,
@@ -204,7 +295,7 @@ public:
     if (ID < NumFunctionIds || ID >= ValueIDValues.size())
       return false;
     WeakVH &OldV = ValueIDValues[ID];
-    if (OldV == NULL) {
+    if (OldV == nullptr) {
       ValueIDValues[ID] = GV;
       return true;
     }
@@ -234,13 +325,13 @@ public:
   }
 
   /// Returns the LLVM integer type with the given number of Bits.  If
-  /// Bits is not a valid PNaCl type, returns NULL.
+  /// Bits is not a valid PNaCl type, returns nullptr.
   Type *getLLVMIntegerType(unsigned Bits) const {

[jvoung (off chromium), 2014/10/06 14:32:17]

can this be removed now?

[Karl, 2014/10/06 21:15:54]

Removed getLLVMIntegerType and getLLVMVectorType.

[jvoung (off chromium), 2014/10/07 15:54:41]

Can it be removed from TypeConverter now too?

     return TypeConverter.getLLVMIntegerType(Bits);
   }
 
   /// Returns the LLVM vector with the given Size and Ty. If not a
-  /// valid PNaCl vector type, returns NULL.
+  /// valid PNaCl vector type, returns nullptr.
   Type *getLLVMVectorType(unsigned Size, Ice::Type Ty) const {
     return TypeConverter.getLLVMVectorType(Size, Ty);
   }
@@ -266,7 +357,7 @@ private:
   // The number of errors reported.
   unsigned NumErrors;
   // The types associated with each type ID.
-  std::vector<Type *> TypeIDValues;
+  std::vector<ExtendedType *> TypeIDValues;
   // The (global) value IDs.
   std::vector<WeakVH> ValueIDValues;
   // Relocatable constants associated with ValueIDValues.
@@ -281,39 +372,67 @@ private:
   // Cached global variable placeholder type. Used for all forward
   // references to global variable addresses.
   Type *GlobalVarPlaceHolderType;
+  // Models an undefined function type signature.
+  Ice::FcnSigType UndefinedFcnSigType;
 
   bool ParseBlock(unsigned BlockID) override;
 
-  /// Reports that type ID is undefined, and then returns
-  /// the void type.
-  Type *reportTypeIDAsUndefined(unsigned ID);
+  /// Reports that type ID is undefined.

[jvoung (off chromium), 2014/10/06 14:32:17]

double vs triple slash consistency

Clarify this is for simple types? Or you could share reportTypeIDAsUndefined()
between the two, but have some parameter that can be used to clarify the error
message.

[Karl, 2014/10/06 21:15:54]

Simplified all type id look ups to go through one lookup function
getTypeByIDAsKind, and one error report function reportBadTypeIDAs.

+  void reportTypeIDAsUndefined(unsigned ID);
+
+  // Reports that type ID as not a simple type.
+  void reportTypeIDNotSimple(unsigned ID, const ExtendedType *Ty);
+
+  // Reports that type ID is undefined.

[jvoung (off chromium), 2014/10/06 14:32:17]

Clarify this is for function signature types?

[Karl, 2014/10/06 21:15:54]

Removed. No longer needed.

+  void reportFcnSigTypeIDAsUndefined(unsigned ID);
+
+  // Reports that type ID is not a function signature.
+  void reportFcnSigTypeIDNotFcnSig(unsigned ID, const ExtendedType *Ty);
 
   /// Reports error about bad call to setTypeID.
-  void reportBadSetTypeID(unsigned ID, Type *Ty);
+  void reportBadSetTypeID(unsigned ID, ExtendedType *Ty);

[Jim Stichnoth, 2014/10/05 15:35:48]

const ExtendedType * ?

[Karl, 2014/10/06 21:15:54]

Added to reportBadTypeIDAs.

 
   // Reports that there is no corresponding ICE type for LLVMTy, and
   // returns ICE::IceType_void.
   Ice::Type convertToIceTypeError(Type *LLVMTy);
 };
 
-Type *TopLevelParser::reportTypeIDAsUndefined(unsigned ID) {
+void TopLevelParser::reportTypeIDAsUndefined(unsigned ID) {
   std::string Buffer;
   raw_string_ostream StrBuf(Buffer);
   StrBuf << "Can't find type for type id: " << ID;
   Error(StrBuf.str());
-  // TODO(kschimpf) Remove error recovery once implementation complete.
-  Type *Ty = TypeConverter.convertToLLVMType(Ice::IceType_void);
-  // To reduce error messages, update type list if possible.
-  if (ID < TypeIDValues.size())
-    TypeIDValues[ID] = Ty;
-  return Ty;
 }
 
-void TopLevelParser::reportBadSetTypeID(unsigned ID, Type *Ty) {
+void TopLevelParser::reportTypeIDNotSimple(unsigned ID,
+                                           const ExtendedType *Ty) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Type id " << ID << " not simple type. Found: " << *Ty;
+  Error(StrBuf.str());
+};
+
+void TopLevelParser::reportFcnSigTypeIDAsUndefined(unsigned ID) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Can't find function signature type for type id: " << ID;
+  Error(StrBuf.str());
+}
+
+void TopLevelParser::reportFcnSigTypeIDNotFcnSig(unsigned ID,
+                                                 const ExtendedType *Ty) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Type id " << ID << " not function signature type. Found:" << *Ty;
+  Error(StrBuf.str());
+}
+
+void TopLevelParser::reportBadSetTypeID(unsigned ID, ExtendedType *Ty) {
   std::string Buffer;
   raw_string_ostream StrBuf(Buffer);
   if (ID >= TypeIDValues.size()) {
-    StrBuf << "Type index " << ID << " out of range: can't install.";
+    StrBuf << "Type index " << ID
+           << " out of range. Can't install type: " << Ty;
   } else {
     // Must be case that index already defined.
     StrBuf << "Type index " << ID << " defined as " << *TypeIDValues[ID]
@@ -380,7 +499,7 @@ protected:
     const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
     if (Values.size() == Size)
       return true;
-    ReportRecordSizeError(Size, RecordName, NULL);
+    ReportRecordSizeError(Size, RecordName, nullptr);
     return false;
   }
 
@@ -418,7 +537,7 @@ protected:
 private:
   /// Generates a record size error. ExpectedSize is the number
   /// of elements expected. RecordName is the name of the kind of
-  /// record that has incorrect size. ContextMessage (if not NULL)
+  /// record that has incorrect size. ContextMessage (if not nullptr)
   /// is appended to "record expects" to describe how ExpectedSize
   /// should be interpreted.
   void ReportRecordSizeError(unsigned ExpectedSize, const char *RecordName,
@@ -477,7 +596,7 @@ private:
 };
 
 void TypesParser::ProcessRecord() {
-  Type *Ty = NULL;
+  ExtendedType *Ty = nullptr;
   const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
   switch (Record.GetCode()) {
   case naclbitc::TYPE_CODE_NUMENTRY:
@@ -490,49 +609,98 @@ void TypesParser::ProcessRecord() {
     // VOID
     if (!isValidRecordSize(0, "Type void"))
       return;
-    Ty = Context->convertToLLVMType(Ice::IceType_void);
+    Ty = new SimpleExtendedType(Ice::IceType_void);
     break;
   case naclbitc::TYPE_CODE_FLOAT:
     // FLOAT
     if (!isValidRecordSize(0, "Type float"))
       return;
-    Ty = Context->convertToLLVMType(Ice::IceType_f32);
+    Ty = new SimpleExtendedType(Ice::IceType_f32);
     break;
   case naclbitc::TYPE_CODE_DOUBLE:
     // DOUBLE
     if (!isValidRecordSize(0, "Type double"))
       return;
-    Ty = Context->convertToLLVMType(Ice::IceType_f64);
+    Ty = new SimpleExtendedType(Ice::IceType_f64);
     break;
   case naclbitc::TYPE_CODE_INTEGER:
     // INTEGER: [width]
     if (!isValidRecordSize(1, "Type integer"))
       return;
-    Ty = Context->getLLVMIntegerType(Values[0]);
-    if (Ty == NULL) {
+    switch (Values[0]) {
+    case 1:
+      Ty = new SimpleExtendedType(Ice::IceType_i1);
+      break;
+    case 8:
+      Ty = new SimpleExtendedType(Ice::IceType_i8);
+      break;
+    case 16:
+      Ty = new SimpleExtendedType(Ice::IceType_i16);
+      break;
+    case 32:
+      Ty = new SimpleExtendedType(Ice::IceType_i32);
+      break;
+    case 64:
+      Ty = new SimpleExtendedType(Ice::IceType_i64);
+      break;
+    default:
+      break;
+    }
+    if (Ty == nullptr) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Type integer record with invalid bitsize: " << Values[0];
       Error(StrBuf.str());
-      // TODO(kschimpf) Remove error recovery once implementation complete.
-      // Fix type so that we can continue.
-      Ty = Context->convertToLLVMType(Ice::IceType_i32);
     }
     break;
   case naclbitc::TYPE_CODE_VECTOR: {
     // VECTOR: [numelts, eltty]
     if (!isValidRecordSize(2, "Type vector"))
       return;
-    Type *BaseTy = Context->getTypeByID(Values[1]);
-    Ty = Context->getLLVMVectorType(Values[0],

[jvoung (off chromium), 2014/10/06 14:32:17]

can getLLVMVectorType be removed now?

[Karl, 2014/10/06 21:15:54]

Done.

-                                    Context->convertToIceType(BaseTy));
-    if (Ty == NULL) {
+    Ice::Type BaseTy = Context->getTypeByID(Values[1]);
+    Ice::SizeT Size = Values[0];
+    switch (BaseTy) {
+    case Ice::IceType_i1:
+      switch (Size) {
+      case 4:
+        Ty = new SimpleExtendedType(Ice::IceType_v4i1);
+        break;
+      case 8:
+        Ty = new SimpleExtendedType(Ice::IceType_v8i1);
+        break;
+      case 16:
+        Ty = new SimpleExtendedType(Ice::IceType_v16i1);
+        break;
+      default:
+        break;
+      }
+      break;
+    case Ice::IceType_i8:
+      if (Size == 16)
+        Ty = new SimpleExtendedType(Ice::IceType_v16i8);
+      break;
+    case Ice::IceType_i16:
+      if (Size == 8)
+        Ty = new SimpleExtendedType(Ice::IceType_v8i16);
+      break;
+    case Ice::IceType_i32:
+      if (Size == 4)
+        Ty = new SimpleExtendedType(Ice::IceType_v4i32);
+      break;
+    case Ice::IceType_f32:
+      if (Size == 4)
+        Ty = new SimpleExtendedType(Ice::IceType_v4f32);
+      break;
+    default:
+      break;
+    }
+
+    if (Ty == nullptr) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Invalid type vector record: <" << Values[0] << " x " << *BaseTy
+      StrBuf << "Invalid type vector record: <" << Values[0] << " x " << BaseTy
              << ">";
       Error(StrBuf.str());
-      Ty = Context->convertToLLVMType(Ice::IceType_void);
     }
     break;
   }
@@ -540,11 +708,26 @@ void TypesParser::ProcessRecord() {
     // FUNCTION: [vararg, retty, paramty x N]
     if (!isValidRecordSizeAtLeast(2, "Type signature"))
       return;
-    SmallVector<Type *, 8> ArgTys;
+    if (Values[0])
+      Error("Function type can't define varargs");
+    FcnSigExtendedType *FcnTy =
+        new FcnSigExtendedType(Context->getTypeByID(Values[1]));
     for (unsigned i = 2, e = Values.size(); i != e; ++i) {
-      ArgTys.push_back(Context->getTypeByID(Values[i]));
+      // Check that type void not used as argument type.
+      // Note: PNaCl restrictions can't be checked until we
+      // know the name, because we have to check for intrinsic signatures.
+      Ice::Type ArgTy = Context->getTypeByID(Values[i]);
+      if (ArgTy == Ice::IceType_void) {
+        std::string Buffer;
+        raw_string_ostream StrBuf(Buffer);
+        StrBuf << "Type for parameter " << (i - 1)
+               << " not valid. Found: " << ArgTy;
+        // TODO(kschimpf) Remove error recovery once implementation complete.
+        ArgTy = Ice::IceType_i32;
+      }
+      FcnTy->appendArgType(ArgTy);
     }
-    Ty = FunctionType::get(Context->getTypeByID(Values[1]), ArgTys, Values[0]);
+    Ty = FcnTy;
     break;
   }
   default:
@@ -552,8 +735,10 @@ void TypesParser::ProcessRecord() {
     return;
   }
   // If Ty not defined, assume error. Use void as filler.
-  if (Ty == NULL)
-    Ty = Context->convertToLLVMType(Ice::IceType_void);
+  // TODO(kschimpf) Remove error recovery once implementation complete.
+  // Fix type so that we can continue.
+  if (Ty == nullptr)
+    Ty = new SimpleExtendedType(Ice::IceType_void);
   Context->setTypeID(NextTypeId++, Ty);
 }
 
@@ -636,7 +821,7 @@ private:
   // installs a global variable (with the initializers) into the list
   // of ValueIDs.
   void installGlobalVar() {
-    Constant *Init = NULL;
+    Constant *Init = nullptr;
     switch (Initializers.size()) {
     case 0:
       Error("No initializer for global variable in global vars block");
@@ -740,7 +925,7 @@ void GlobalsParser::ProcessRecord() {
     if (!isValidRecordSizeInRange(1, 2, "Globals reloc"))
       return;
     Constant *BaseVal = Context->getOrCreateGlobalVarRef(Values[0]);
-    if (BaseVal == NULL) {
+    if (BaseVal == nullptr) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Can't find global relocation value: " << Values[0];
@@ -956,7 +1141,7 @@ private:
     uint32_t LocalIndex = NextLocalInstIndex - CachedNumGlobalValueIDs;
     if (LocalIndex < LocalOperands.size()) {
       Ice::Operand *Op = LocalOperands[LocalIndex];
-      if (Op != NULL) {
+      if (Op != nullptr) {
         if (Ice::Variable *Var = dyn_cast<Ice::Variable>(Op)) {
           if (Var->getType() == Ty) {
             ++NextLocalInstIndex;
@@ -1007,7 +1192,7 @@ private:
       report_fatal_error("Unable to continue");
     }
     Ice::Operand *Op = LocalOperands[LocalIndex];
-    if (Op == NULL) {
+    if (Op == nullptr) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Value index " << Index << " not defined!";
@@ -1033,7 +1218,7 @@ private:
 
     // If element not defined, set it.
     Ice::Operand *OldOp = LocalOperands[LocalIndex];
-    if (OldOp == NULL) {
+    if (OldOp == nullptr) {
       LocalOperands[LocalIndex] = Op;
       return;
     }
@@ -1473,7 +1658,7 @@ void FunctionParser::ProcessRecord() {
     if (!isValidRecordSize(3, "function block cast"))
       return;
     Ice::Operand *Src = getRelativeOperand(Values[0], BaseIndex);
-    Type *CastType = Context->getTypeByID(Values[1]);
+    Ice::Type CastType = Context->getTypeByID(Values[1]);
     Instruction::CastOps LLVMCastOp;
     Ice::InstCast::OpKind CastKind;
     if (!naclbitc::DecodeCastOpcode(Values[2], LLVMCastOp) ||
@@ -1484,18 +1669,18 @@ void FunctionParser::ProcessRecord() {
       Error(StrBuf.str());
       return;
     }
-    Type *SrcType = Context->convertToLLVMType(Src->getType());
-    if (!CastInst::castIsValid(LLVMCastOp, SrcType, CastType)) {
+    Ice::Type SrcType = Src->getType();
+    if (!CastInst::castIsValid(LLVMCastOp, Context->convertToLLVMType(SrcType),
+                               Context->convertToLLVMType(CastType))) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Illegal cast: " << Instruction::getOpcodeName(LLVMCastOp)
-             << " " << *SrcType << " to " << *CastType;
+             << " " << SrcType << " to " << CastType;
       Error(StrBuf.str());
       return;
     }
-    CurrentNode->appendInst(Ice::InstCast::create(
-        Func, CastKind, getNextInstVar(Context->convertToIceType(CastType)),
-        Src));
+    CurrentNode->appendInst(
+        Ice::InstCast::create(Func, CastKind, getNextInstVar(CastType), Src));
     break;
   }
   case naclbitc::FUNC_CODE_INST_VSELECT: {
@@ -1678,7 +1863,7 @@ void FunctionParser::ProcessRecord() {
     if (Values.size() == 1) {
       // BR: [bb#]
       Ice::CfgNode *Block = getBranchBasicBlock(Values[0]);
-      if (Block == NULL)
+      if (Block == nullptr)
         return;
       CurrentNode->appendInst(Ice::InstBr::create(Func, Block));
     } else {
@@ -1696,7 +1881,7 @@ void FunctionParser::ProcessRecord() {
       }
       Ice::CfgNode *ThenBlock = getBranchBasicBlock(Values[0]);
       Ice::CfgNode *ElseBlock = getBranchBasicBlock(Values[1]);
-      if (ThenBlock == NULL || ElseBlock == NULL)
+      if (ThenBlock == nullptr || ElseBlock == nullptr)
         return;
       CurrentNode->appendInst(
           Ice::InstBr::create(Func, Cond, ThenBlock, ElseBlock));
@@ -1715,8 +1900,7 @@ void FunctionParser::ProcessRecord() {
     // already frozen when the problem was noticed.
     if (!isValidRecordSizeAtLeast(4, "function block switch"))
       return;
-    Ice::Type CondTy =
-        Context->convertToIceType(Context->getTypeByID(Values[0]));
+    Ice::Type CondTy = Context->getTypeByID(Values[0]);
     if (!Ice::isScalarIntegerType(CondTy)) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
@@ -1784,7 +1968,7 @@ void FunctionParser::ProcessRecord() {
       Error(StrBuf.str());
       return;
     }
-    Ice::Type Ty = Context->convertToIceType(Context->getTypeByID(Values[0]));
+    Ice::Type Ty = Context->getTypeByID(Values[0]);
     if (Ty == Ice::IceType_void) {
       Error("Phi record using type void not allowed");
       return;
@@ -1835,7 +2019,7 @@ void FunctionParser::ProcessRecord() {
       return;
     unsigned Alignment;
     extractAlignment("Load", Values[1], Alignment);
-    Ice::Type Ty = Context->convertToIceType(Context->getTypeByID(Values[2]));
+    Ice::Type Ty = Context->getTypeByID(Values[2]);
     if (!isValidLoadStoreAlignment(Alignment, Ty, "Load"))
       return;
     CurrentNode->appendInst(
@@ -1897,11 +2081,11 @@ void FunctionParser::ProcessRecord() {
     uint32_t CalleeIndex = convertRelativeToAbsIndex(Values[1], BaseIndex);
     Ice::Operand *Callee = getOperand(CalleeIndex);
     Ice::Type ReturnType = Ice::IceType_void;
-    const Ice::Intrinsics::FullIntrinsicInfo *IntrinsicInfo = NULL;
+    const Ice::Intrinsics::FullIntrinsicInfo *IntrinsicInfo = nullptr;
     if (Record.GetCode() == naclbitc::FUNC_CODE_INST_CALL) {
       Function *Fcn =
           dyn_cast<Function>(Context->getGlobalValueByID(CalleeIndex));
-      if (Fcn == NULL) {
+      if (Fcn == nullptr) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
         StrBuf << "Function call to non-function: " << *Callee;
@@ -1928,14 +2112,15 @@ void FunctionParser::ProcessRecord() {
         }
       }
     } else {
-      ReturnType = Context->convertToIceType(Context->getTypeByID(Values[2]));
+      ReturnType = Context->getTypeByID(Values[2]);
     }
 
     // Create the call instruction.
-    Ice::Variable *Dest =
-        (ReturnType == Ice::IceType_void) ? NULL : getNextInstVar(ReturnType);
+    Ice::Variable *Dest = (ReturnType == Ice::IceType_void)
+                              ? nullptr
+                              : getNextInstVar(ReturnType);
     Ice::SizeT NumParams = Values.size() - ParamsStartIndex;
-    Ice::InstCall *Inst = NULL;
+    Ice::InstCall *Inst = nullptr;
     if (IntrinsicInfo) {
       Inst =
           Ice::InstIntrinsicCall::create(Func, NumParams, Dest, Callee,
@@ -1999,8 +2184,7 @@ void FunctionParser::ProcessRecord() {
     // FORWARDTYPEREF: [opval, ty]
     if (!isValidRecordSize(2, "function block forward type ref"))
       return;
-    setOperand(Values[0], createInstVar(Context->convertToIceType(
-                              Context->getTypeByID(Values[1]))));
+    setOperand(Values[0], createInstVar(Context->getTypeByID(Values[1])));
     break;
   }
   default:
@@ -2049,8 +2233,7 @@ void ConstantsParser::ProcessRecord() {
     // SETTYPE: [typeid]
     if (!isValidRecordSize(1, "constants block set type"))
       return;
-    NextConstantType =
-        Context->convertToIceType(Context->getTypeByID(Values[0]));
+    NextConstantType = Context->getTypeByID(Values[0]);
     if (NextConstantType == Ice::IceType_void)
       Error("constants block set type not allowed for void type");
     return;
@@ -2255,7 +2438,7 @@ private:
 
 void ModuleValuesymtabParser::setValueName(uint64_t Index, StringType &Name) {
   Value *V = Context->getGlobalValueByID(Index);
-  if (V == NULL) {
+  if (V == nullptr) {
     std::string Buffer;
     raw_string_ostream StrBuf(Buffer);
     StrBuf << "Invalid global address ID in valuesymtab: " << Index;
@@ -2319,15 +2502,7 @@ void ModuleParser::ProcessRecord() {
     // FUNCTION:  [type, callingconv, isproto, linkage]
     if (!isValidRecordSize(4, "Function heading"))
       return;
-    Type *Ty = Context->getTypeByID(Values[0]);
-    FunctionType *FTy = dyn_cast<FunctionType>(Ty);
-    if (FTy == NULL) {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Function heading expects function type. Found: " << Ty;
-      Error(StrBuf.str());
-      return;
-    }
+    const Ice::FcnSigType &Ty = Context->getFcnSigTypeByID(Values[0]);
     CallingConv::ID CallingConv;
     if (!naclbitc::DecodeCallingConv(Values[1], CallingConv)) {
       std::string Buffer;
@@ -2345,7 +2520,14 @@ void ModuleParser::ProcessRecord() {
       Error(StrBuf.str());
       return;
     }
-    Function *Func = Function::Create(FTy, Linkage, "", Context->getModule());
+    SmallVector<Type *, 8> ArgTys;
+    for (Ice::Type ArgType : Ty.getArgList()) {
+      ArgTys.push_back(Context->convertToLLVMType(ArgType));
+    }
+    Function *Func = Function::Create(
+        FunctionType::get(Context->convertToLLVMType(Ty.getReturnType()),
+                          ArgTys, false),
+        Linkage, "", Context->getModule());
     Func->setCallingConv(CallingConv);
     if (Values[2] == 0)
       Context->setNextValueIDAsImplementedFunction();