Start processing function blocks in Subzero.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 7f9c514542a73cbebec36d9cb3a2bbd9ad15515b..e66cd84c7b0a547a2831f458c5bbdd347782cced 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -14,6 +14,13 @@
 
 #include "PNaClTranslator.h"
 #include "IceCfg.h"
+#include "IceCfgNode.h"
+#include "IceClFlags.h"
+#include "IceDefs.h"
+#include "IceInst.h"
+#include "IceOperand.h"
+#include "IceTypeConverter.h"
+#include "llvm/Analysis/NaCl/PNaClABITypeChecker.h"
 #include "llvm/Bitcode/NaCl/NaClBitcodeDecoders.h"
 #include "llvm/Bitcode/NaCl/NaClBitcodeHeader.h"
 #include "llvm/Bitcode/NaCl/NaClBitcodeParser.h"
@@ -33,28 +40,42 @@ using namespace llvm;
 
 namespace {
 
+// TODO(kschimpf) Remove error recovery once implementation complete.
+static cl::opt<bool> AllowErrorRecovery(
+    "allow-pnacl-reader-error-recovery",
+    cl::desc("Allow error recovery when reading PNaCl bitcode."),
+    cl::init(false));
+
 // Top-level class to read PNaCl bitcode files, and translate to ICE.
 class TopLevelParser : public NaClBitcodeParser {
   TopLevelParser(const TopLevelParser &) LLVM_DELETED_FUNCTION;
   TopLevelParser &operator=(const TopLevelParser &) LLVM_DELETED_FUNCTION;
 
 public:
-  TopLevelParser(const std::string &InputName, NaClBitcodeHeader &Header,
+  TopLevelParser(Ice::Translator &Translator,
+                 const std::string &InputName, NaClBitcodeHeader &Header,
                  NaClBitstreamCursor &Cursor, bool &ErrorStatus)
       : NaClBitcodeParser(Cursor),
+        Translator(Translator),
         Mod(new Module(InputName, getGlobalContext())), Header(Header),
+        TypeConverter(getLLVMContext()),
         ErrorStatus(ErrorStatus), NumErrors(0), NumFunctionIds(0),
-        GlobalVarPlaceHolderType(Type::getInt8Ty(getLLVMContext())) {
+        NumFunctionBlocks(0),
+        GlobalVarPlaceHolderType(convertToLLVMType(Ice::IceType_i8)) {
     Mod->setDataLayout(PNaClDataLayout);
   }
 
   virtual ~TopLevelParser() {}
   LLVM_OVERRIDE;
 
+  Ice::Translator &getTranslator() { return Translator; }
+
   virtual bool Error(const std::string &Message) LLVM_OVERRIDE {

[Jim Stichnoth, 2014/07/21 22:25:22]

Should be error() not Error() according to coding style.

Also, can you document what the return value is supposed to mean?

[Karl, 2014/07/23 20:22:20]

This is an inherited method, and must follow the conventions it has. Did add
comments.

     ErrorStatus = true;
     ++NumErrors;
-    return NaClBitcodeParser::Error(Message);
+    bool ReturnValue = NaClBitcodeParser::Error(Message);
+    if (!AllowErrorRecovery) report_fatal_error("Unable to continue");

[Jim Stichnoth, 2014/07/21 22:25:21]

I don't like "if (cond) stmt;" on a single line, and I suspect clang-format
doesn't like it either. :)

[Karl, 2014/07/23 20:22:21]

Fixed by running through clang-format.

+    return ReturnValue;
   }
 
   /// Returns the number of errors found while parsing the bitcode
@@ -104,6 +125,16 @@ public:
     DefiningFunctionsList.push_back(ValueIDValues.size());
   }
 
+  /// Returns the value id that should be associated with the the
+  /// current function block. Increments internal counters during call
+  /// so that it will be in correct position for next function block.
+  unsigned getNextFunctionBlockValueID() {
+    if (DefiningFunctionsList.size() <= NumFunctionBlocks)

[jvoung (off chromium), 2014/07/23 16:10:17]

nit: flipping the comparison and using >= seems to read better (the error
message is "more function blocks than...")

[Karl, 2014/07/23 20:22:21]

Done.

+      report_fatal_error(
+          "More function blocks than defined function addresses");
+    return DefiningFunctionsList[NumFunctionBlocks++];
+  }
+
   /// Returns the LLVM IR value associatd with the global value ID.
   Value *getGlobalValueByID(unsigned ID) const {
     if (ID >= ValueIDValues.size())
@@ -162,11 +193,46 @@ public:
     return true;
   }
 
+  /// Returns the corresponding ICE type for LLVMTy.
+  Ice::Type convertToIceType(Type *LLVMTy) {
+    Ice::Type IceTy = TypeConverter.convertToIceType(LLVMTy);
+    if (IceTy == Ice::IceType_NUM) {
+      return convertToIceTypeError(LLVMTy);
+    }
+    return IceTy;
+  }
+
+  /// Returns the corresponding LLVM type for IceTy.
+  Type *convertToLLVMType(Ice::Type IceTy) const {
+    return TypeConverter.convertToLLVMType(IceTy);
+  }
+
+  /// Returns the LLVM integer type with the given number of Bits.  If
+  /// Bits is not a valid PNaCl type, returns NULL.
+  Type *getLLVMIntegerType(unsigned Bits) const {
+    return TypeConverter.getLLVMIntegerType(Bits);
+  }
+
+  /// Returns the LLVM vector with the given Size and Ty. If not a
+  /// valid PNaCl vector type, returns NULL.
+  Type *getLLVMVectorType(unsigned Size, Ice::Type Ty) const {
+    return TypeConverter.getLLVMVectorType(Size, Ty);
+  }
+
+  /// Returns the model for pointer types in ICE.
+  Ice::Type getIcePointerType() const {
+    return TypeConverter.getIcePointerType();
+  }
+
 private:
+  // The translator associated with the parser.
+  Ice::Translator &Translator;
   // The parsed module.
   OwningPtr<Module> Mod;
   // The bitcode header.
   NaClBitcodeHeader &Header;
+  // Converter between LLVM and ICE types.
+  Ice::TypeConverter TypeConverter;
   // The exit status that should be set to true if an error occurs.
   bool &ErrorStatus;
   // The number of errors reported.
@@ -177,6 +243,8 @@ private:
   std::vector<WeakVH> ValueIDValues;
   // The number of function IDs.
   unsigned NumFunctionIds;
+  // The number of function blocks (processed so far).
+  unsigned NumFunctionBlocks;
   // The list of value IDs (in the order found) of defining function
   // addresses.
   std::vector<unsigned> DefiningFunctionsList;
@@ -192,6 +260,10 @@ private:
 
   /// Reports error about bad call to setTypeID.
   void reportBadSetTypeID(unsigned ID, Type *Ty);
+
+  // Reports that there is no corresponding ICE type for LLVMTy.
+  // returns ICE::IceType_void.
+  Ice::Type convertToIceTypeError(Type *LLVMTy);
 };
 
 Type *TopLevelParser::reportTypeIDAsUndefined(unsigned ID) {
@@ -199,7 +271,8 @@ Type *TopLevelParser::reportTypeIDAsUndefined(unsigned ID) {
   raw_string_ostream StrBuf(Buffer);
   StrBuf << "Can't find type for type id: " << ID;
   Error(StrBuf.str());
-  Type *Ty = Type::getVoidTy(getLLVMContext());
+  // 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;
@@ -219,6 +292,14 @@ void TopLevelParser::reportBadSetTypeID(unsigned ID, Type *Ty) {
   Error(StrBuf.str());
 }
 
+Ice::Type TopLevelParser::convertToIceTypeError(Type *LLVMTy) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Invalid LLVM type: " << *LLVMTy;
+  Error(StrBuf.str());
+  return Ice::IceType_void;
+}
+
 // Base class for parsing blocks within the bitcode file.  Note:
 // Because this is the base class of block parsers, we generate error
 // messages if ParseBlock or ParseRecord is not overridden in derived
@@ -240,6 +321,11 @@ protected:
       : NaClBitcodeParser(BlockID, EnclosingParser),
         Context(EnclosingParser->Context) {}
 
+  // Gets the translator associated with the bitcode parser.
+  Ice::Translator &getTranslator() {
+    return Context->getTranslator();
+  }
+
   // Generates an error Message with the bit address prefixed to it.
   virtual bool Error(const std::string &Message) LLVM_OVERRIDE {
     uint64_t Bit = Record.GetStartBit() + Context->getHeaderSize() * 8;
@@ -325,6 +411,7 @@ bool BlockParserBaseClass::ParseBlock(unsigned BlockID) {
   raw_string_ostream StrBuf(Buffer);
   StrBuf << "Don't know how to parse block id: " << BlockID;
   Error(StrBuf.str());
+  // TODO(kschimpf) Remove error recovery once implementation complete.
   SkipBlock();
   return false;
 }
@@ -367,33 +454,52 @@ void TypesParser::ProcessRecord() {
     // VOID
     if (checkRecordSize(0, "Type void"))
       break;
-    Ty = Type::getVoidTy(Context->getLLVMContext());
+    Ty = Context->convertToLLVMType(Ice::IceType_void);
     break;
   case naclbitc::TYPE_CODE_FLOAT:
     // FLOAT
     if (checkRecordSize(0, "Type float"))
       break;
-    Ty = Type::getFloatTy(Context->getLLVMContext());
+    Ty = Context->convertToLLVMType(Ice::IceType_f32);
     break;
   case naclbitc::TYPE_CODE_DOUBLE:
     // DOUBLE
     if (checkRecordSize(0, "Type double"))
       break;
-    Ty = Type::getDoubleTy(Context->getLLVMContext());
+    Ty = Context->convertToLLVMType(Ice::IceType_f64);
     break;
   case naclbitc::TYPE_CODE_INTEGER:
     // INTEGER: [width]
     if (checkRecordSize(1, "Type integer"))
       break;
-    Ty = IntegerType::get(Context->getLLVMContext(), Values[0]);
-    // TODO(kschimpf) Check if size is legal.
+    Ty = Context->getLLVMIntegerType(Values[0]);
+    if (Ty == NULL) {
+      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:
+  case naclbitc::TYPE_CODE_VECTOR: {
     // VECTOR: [numelts, eltty]
     if (checkRecordSize(2, "Type vector"))
       break;
-    Ty = VectorType::get(Context->getTypeByID(Values[1]), Values[0]);
+    Type *BaseTy = Context->getTypeByID(Values[1]);
+    Ty = Context->getLLVMVectorType(Values[0],
+                                    Context->convertToIceType(BaseTy));
+    if (Ty == NULL) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Invalid type vector record: <" << Values[0]
+             << " x " << *BaseTy << ">";
+      Error(StrBuf.str());
+      Ty = Context->convertToLLVMType(Ice::IceType_void);
+    }
     break;
+  }
   case naclbitc::TYPE_CODE_FUNCTION: {
     // FUNCTION: [vararg, retty, paramty x N]
     if (checkRecordSizeAtLeast(2, "Type signature"))
@@ -411,7 +517,7 @@ void TypesParser::ProcessRecord() {
   }
   // If Ty not defined, assume error. Use void as filler.
   if (Ty == NULL)
-    Ty = Type::getVoidTy(Context->getLLVMContext());
+    Ty = Context->convertToLLVMType(Ice::IceType_void);
   Context->setTypeID(NextTypeId++, Ty);
 }
 
@@ -474,6 +580,7 @@ private:
         StrBuf << "s";
       StrBuf << ". Found: " << Initializers.size();
       Error(StrBuf.str());
+      // TODO(kschimpf) Remove error recovery once implementation complete.
       // Fix up state so that we can continue.
       InitializersNeeded = Initializers.size();
       installGlobalVar();
@@ -573,7 +680,7 @@ void GlobalsParser::ProcessRecord() {
       return;
     reserveInitializer("Globals zerofill");
     Type *Ty =
-        ArrayType::get(Type::getInt8Ty(Context->getLLVMContext()), Values[0]);
+        ArrayType::get(Context->convertToLLVMType(Ice::IceType_i8), Values[0]);
     Constant *Zero = ConstantAggregateZero::get(Ty);
     Initializers.push_back(Zero);
     break;
@@ -604,7 +711,7 @@ void GlobalsParser::ProcessRecord() {
       Error(StrBuf.str());
       return;
     }
-    Type *IntPtrType = IntegerType::get(Context->getLLVMContext(), 32);
+    Type *IntPtrType = Context->convertToLLVMType(Context->getIcePointerType());
     Constant *Val = ConstantExpr::getPtrToInt(BaseVal, IntPtrType);
     if (Values.size() == 2) {
       Val = ConstantExpr::getAdd(Val, ConstantInt::get(IntPtrType, Values[1]));
@@ -685,6 +792,395 @@ void ValuesymtabParser::ProcessRecord() {
   return;
 }
 
+/// Parses function blocks in the bitcode file.
+class FunctionParser : public BlockParserBaseClass {
+public:
+  FunctionParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser)

[Jim Stichnoth, 2014/07/21 22:25:22]

Disable default copy ctor etc.

[Karl, 2014/07/23 20:22:21]

Done.

+      : BlockParserBaseClass(BlockID, EnclosingParser),
+        Func(new Ice::Cfg(getTranslator().getContext())),
+        CurrentBbIndex(0),
+        FcnId(Context->getNextFunctionBlockValueID()),
+        LLVMFunc(cast<Function>(Context->getGlobalValueByID(FcnId))),
+        CachedNumGlobalValueIDs(Context->getNumGlobalValueIDs()),
+        InstIsTerminating(false)
+  {
+    Func->setFunctionName(LLVMFunc->getName());
+    Func->setReturnType(Context->convertToIceType(LLVMFunc->getReturnType()));
+    Func->setInternal(LLVMFunc->hasInternalLinkage());
+    CurrentNode = InstallNextBasicBlock();
+    for (Function::const_arg_iterator ArgI = LLVMFunc->arg_begin(),
+                                      ArgE = LLVMFunc->arg_end();
+         ArgI != ArgE; ++ArgI) {
+      Func->addArg(NextInstVar(Context->convertToIceType(ArgI->getType())));
+    }
+  }
+
+  ~FunctionParser() LLVM_OVERRIDE;
+
+private:
+  // Timer for reading function bitcode and converting to ICE.
+  Ice::Timer TConvert;
+  // The corresponding ICE function defined by the function block.
+  Ice::Cfg *Func;

[jvoung (off chromium), 2014/07/23 16:10:17]

Smart pointer for Func?
What deletes Func?

I guess if it's a smart pointer, then translateFcn() must be done before the
Func/FunctionParser is destroyed.

[Karl, 2014/07/23 20:22:21]

Yes. We call translateFcn with it in the destructor, which takes over ownership.
Updated comment in IceTranslator.h to clarify that ownership is being taken.

[jvoung (off chromium), 2014/07/24 19:01:44]

Okay.

Something about running translateFcn() in the dtor, doesn't seem clean (a bit
unexpected?). I guess we're not using c++ exceptions, so that eliminates the
possibiilty of wanting to abort and clean up (w/ the dtor) w/out doing the
translation.

[Karl, 2014/07/25 21:49:03]

Decided to move the cleanup/call to the ExitBlock method, so that we don't need
to worry about c++ exceptions.

+  // The index to the current basic block being built.
+  uint32_t CurrentBbIndex;
+  // The basic block being built.
+  Ice::CfgNode *CurrentNode;
+  // The ID for the function.
+  unsigned FcnId;
+  // The corresponding llvm function.

[Jim Stichnoth, 2014/07/21 22:25:22]

LLVM :)

[Karl, 2014/07/23 20:22:21]

Done.

+  Function *LLVMFunc;
+  // Holds variables local to the function block.
+  std::vector<Ice::Operand *> LocalVars;

[Jim Stichnoth, 2014/07/21 22:25:21]

This should probably be a vector of Variable*, not Operand*.

Also, could you use Func->getVariables() instead?

[Karl, 2014/07/23 20:22:21]

This vector is used to convert indices from the bitcode file to corresponding
operands (including forward references which definitely would not be in
Func->getVariables()). Fixing name to LocalOperands (to be consistent with
vector contents) and adding comment to clarify.

[jvoung (off chromium), 2014/07/24 19:01:44]

It can still be a std::vector<Ice::Variable *> (does the subtyping work out)?

[Jim Stichnoth, 2014/07/24 19:55:22]

What I understood from Karl is that ultimately this will indeed contain
Operands, i.e. Variables plus Constants, even though the current code only
supplies Variables.  Is that correct?

[Karl, 2014/07/25 21:49:03]

That is correct.

+  // Holds the list of basic blocks defined for the function.
+  std::vector<Ice::CfgNode*> Nodes;

[Jim Stichnoth, 2014/07/21 22:25:21]

I don't think you need this, since it should be available as Func->getNodes().

[Karl, 2014/07/23 20:22:21]

Done.

+  // Holds the dividing point between local and global absolute value indices.
+  uint32_t CachedNumGlobalValueIDs;
+  // True if the last processed instruction was a terminating
+  // instruction.
+  bool InstIsTerminating;
+
+  virtual void ProcessRecord() LLVM_OVERRIDE;
+
+  // Creates and appends a new basic block to the list of basic blocks.
+  Ice::CfgNode *InstallNextBasicBlock() {
+    std::string Buffer;
+    raw_string_ostream StrBuf(Buffer);
+    // TODO(kschimpf): Make (basic block) nodes make up the name if needed.
+    StrBuf << "bb" << Nodes.size();
+    Ice::CfgNode *Node = Func->makeNode(StrBuf.str());
+    Nodes.push_back(Node);
+    return Node;
+  }
+
+  // Returns the Index-th basic block in the list of basic blocks.
+  Ice::CfgNode *GetBasicBlock(uint32_t Index) {
+    if (Index >= Nodes.size()) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Reference to basic block " << Index
+             << " not found. Must be less than " << Nodes.size();
+      Error(StrBuf.str());
+      // TODO(kschimpf) Remove error recovery once implementation complete.
+      Index = 0;
+    }
+    return Nodes[Index];
+  }
+
+  // Generates the next available local variable using the given
+  // type.  Note: if Ty is void, this function returns NULL.
+  Ice::Variable *NextInstVar(Ice::Type Ty) {
+    if (Ty == Ice::IceType_void)
+      return NULL;
+    Ice::Variable *Var = Func->makeVariable(Ty, CurrentNode);
+    LocalVars.push_back(Var);
+    return Var;
+  }
+
+  // Converts a relative index (to the next instruction to be read) to
+  // an absolute value index.
+  uint32_t convertRelativeToAbsIndex(int32_t Id) {
+    int32_t AbsNextId = CachedNumGlobalValueIDs + LocalVars.size();
+    if (Id > 0 && AbsNextId < static_cast<uint32_t>(Id)) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Invalid relative value id: " << Id
+             << " (must be <= " << AbsNextId << ")";
+      Error(StrBuf.str());
+      // TODO(kschimpf) Remove error recovery once implementation complete.
+      return 0;
+    }
+    return AbsNextId - Id;
+  }
+
+  // Returns the value referenced by the given value Index.
+  Ice::Operand *getOperand(uint32_t Index) {
+    if (Index < CachedNumGlobalValueIDs) {
+      // TODO(kschimpf): Define implementation.
+      report_fatal_error("getOperand of global addresses not implemented");
+    }
+    uint32_t LocalIndex = Index - CachedNumGlobalValueIDs;
+    if (LocalIndex >= LocalVars.size()) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Value index " << Index << " out of range. Must be less than "
+             << (LocalVars.size() + CachedNumGlobalValueIDs);
+      Error(StrBuf.str());
+      report_fatal_error("Unable to continue");
+    }
+    return LocalVars[LocalIndex];
+  }
+
+  // Checks if integer arithmetic Op, for type OpTy, is valid.
+  // Returns false if valid. Otherwise generates error message and

[Jim Stichnoth, 2014/07/21 22:25:22]

I find the "return false if valid, true if invalid" semantics counterintuitive. 
It's understandable when a Unix-style return code is an int where 0 means
success and nonzero means one of many possible failure codes.  But for a bool
return type, I think true should be success (unless the function name suggests
otherwise, like checkIfNotIntArithmeticOp()).

Also, consider naming these isValidFoo(), to align with the various
PNaClABITypeChecker methods they are based on.

This applies here and several other places.

[Karl, 2014/07/23 20:22:20]

Changed the context of boolean functions (when possible) to return true if the
condition is met, and false otherwise. The exceptions are for inherited
functions which use the other convention (i.e. return true when error occurs).

+  // returns true.
+  bool checkIfIntArithmeticOp(Ice::InstArithmetic::OpKind Op, Ice::Type OpTy) {
+    if (!PNaClABITypeChecker::isValidIntArithmeticType(
+            Context->convertToLLVMType(OpTy))) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << Ice::InstArithmetic::getOpName(Op)
+             << ": Invalid integer arithmetic type: " << OpTy;
+      return Error(StrBuf.str());
+    }
+    return false;
+  }
+
+  // Checks if integer (or vector of integers) arithmetic Op, for type
+  // OpTy, is valid.  Returns false if valid. Otherwise generates
+  // error message and returns true.
+  bool checkIfIntOrIntVectorArithOp(Ice::InstArithmetic::OpKind Op,
+                                    Ice::Type OpTy) {
+    Ice::Type BaseTy = OpTy;
+    if (Ice::isVectorType(OpTy)) {
+      if (!PNaClABITypeChecker::isValidVectorType(
+              Context->convertToLLVMType(OpTy))) {
+        std::string Buffer;
+        raw_string_ostream StrBuf(Buffer);
+        StrBuf << Ice::InstArithmetic::getOpName(Op)
+               << ": invalid vector type: " << OpTy;
+        return Error(StrBuf.str());
+      }
+      BaseTy = Ice::typeElementType(OpTy);
+    }
+    if (Ice::isIntegerType(BaseTy)) {
+      if (PNaClABITypeChecker::isValidIntArithmeticType(
+              Context->convertToLLVMType(BaseTy))) {
+        return false;
+      }
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << Ice::InstArithmetic::getOpName(Op)
+             << ": Invalid integer type: " << OpTy;
+      return Error(StrBuf.str());
+    } else {

[Jim Stichnoth, 2014/07/21 22:25:21]

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

[Karl, 2014/07/23 20:22:20]

Done.

+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << Ice::InstArithmetic::getOpName(Op)
+             << ": Expects integer type. Found: " << OpTy;
+      return Error(StrBuf.str());
+    }
+    return false;

[Jim Stichnoth, 2014/07/21 22:25:21]

This return statement is unreachable, right?

[Karl, 2014/07/23 20:22:20]

Yes. But I've refactored the code to better match the "isValid" form of this.

+  }
+
+  // Checks if floating arithmetic Op, for type OpTy, is valid.
+  // Returns false if valid. Otherwise generates an error message and
+  // returns true.
+  bool checkIfFloatOrVectorFloatArithOp(Ice::InstArithmetic::OpKind Op,
+                                        Ice::Type OpTy) {
+    Ice::Type BaseTy = OpTy;
+    if (Ice::isVectorType(OpTy)) {
+      if (!PNaClABITypeChecker::isValidVectorType(
+              Context->convertToLLVMType(OpTy))) {
+        std::string Buffer;
+        raw_string_ostream StrBuf(Buffer);
+        StrBuf << Ice::InstArithmetic::getOpName(Op)
+               << ": invalid vector type: " << OpTy;
+        return Error(StrBuf.str());
+      }
+      BaseTy = Ice::typeElementType(OpTy);
+    }
+    if (Ice::isFloatingType(BaseTy)) {

[jvoung (off chromium), 2014/07/22 18:40:18]

not isFloatingType ?

[Karl, 2014/07/23 20:22:21]

Yes. Fixed.

+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << Ice::InstArithmetic::getOpName(Op)
+             << ": Expects floating point. Found " << OpTy;
+      return Error(StrBuf.str());
+    }
+    if (!PNaClABITypeChecker::isValidScalarType(

[jvoung (off chromium), 2014/07/22 18:40:18]

When will this end up begin checked, if isFloatingType() already checks things?

[Karl, 2014/07/23 20:22:20]

At the moment yes. However, the question is whether we should call the PNaCl ABI
checking fucntions to handle any changes we may add (such as a larger floating
point type). However, I agree that this is unlikely. Removing.

+            Context->convertToLLVMType(BaseTy))) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << Ice::InstArithmetic::getOpName(Op)
+             << ": type not allowed: " << OpTy;
+      return Error(StrBuf.str());
+    }
+    return false;
+  }
+
+  /// Takes the PNaCl bitcode binary operator Opcode, and the opcode
+  /// type Ty, and sets Op to the corresponding ICE binary
+  /// opcode. Returns false if able to convert, true otherwise.
+  bool convertBinopOpcode(unsigned Opcode,
+                          Ice::Type Ty,
+                          Ice::InstArithmetic::OpKind &Op) {
+    Instruction::BinaryOps LLVMOpcode;
+    if (!naclbitc::DecodeBinaryOpcode(
+            Opcode, Context->convertToLLVMType(Ty), LLVMOpcode)) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Binary opcode not understood: " << Opcode;
+      Error(StrBuf.str());
+      Op = Ice::InstArithmetic::Add;
+      return true;
+    }
+    switch (LLVMOpcode) {
+    default: {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Binary opcode not understood: " << Opcode;
+      Error(StrBuf.str());
+      // TODO(kschimpf) Remove error recovery once implementation complete.
+      Op = Ice::InstArithmetic::Add;
+      return true;
+    }
+    case Instruction::Add:
+      Op = Ice::InstArithmetic::Add;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::FAdd:
+      Op = Ice::InstArithmetic::Fadd;
+      return checkIfFloatOrVectorFloatArithOp(Op, Ty);
+    case Instruction::Sub:
+      Op = Ice::InstArithmetic::Sub;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::FSub:
+      Op = Ice::InstArithmetic::Fsub;
+      return checkIfFloatOrVectorFloatArithOp(Op, Ty);
+    case Instruction::Mul:
+      Op = Ice::InstArithmetic::Mul;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::FMul:
+      Op = Ice::InstArithmetic::Fmul;
+      return checkIfFloatOrVectorFloatArithOp(Op, Ty);
+    case Instruction::UDiv:
+      Op = Ice::InstArithmetic::Udiv;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::SDiv:
+      Op = Ice::InstArithmetic::Sdiv;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::FDiv:
+      Op = Ice::InstArithmetic::Fdiv;
+      return checkIfFloatOrVectorFloatArithOp(Op, Ty);
+    case Instruction::URem:
+      Op = Ice::InstArithmetic::Urem;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::SRem:
+      Op = Ice::InstArithmetic::Srem;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::FRem:
+      Op = Ice::InstArithmetic::Frem;
+      return checkIfFloatOrVectorFloatArithOp(Op, Ty);
+    case Instruction::Shl:
+      Op = Ice::InstArithmetic::Shl;
+      return checkIfIntArithmeticOp(Op, Ty);

[jvoung (off chromium), 2014/07/23 16:10:17]

It should be possible to Shl on an integer vector as well?

See tests_lit/llvm2ice_tests/vector-arith.ll

[Karl, 2014/07/23 20:22:21]

I see. I'm slightly confused with the semantics. Renaming (and changing
semantics) to "isValidIntOrIntVectorLogicalOp". The difference between
Arithmetic and Logical is that i1 is also allowed.

[jvoung (off chromium), 2014/07/24 19:01:44]

I think you mean changing to isValidIntOrIntVectorArithOp (which is what you
changed this shift op to check).  This allows each vector element to be shifted
by a different amount (specified by the second vector's elements).

Good catch with the And/Or/Xor, changing that to check "logical" types.

[Karl, 2014/07/25 21:49:03]

Yes. My comment was incorrect. However the code looks right.

+    case Instruction::LShr:
+      Op = Ice::InstArithmetic::Lshr;
+      return checkIfIntArithmeticOp(Op, Ty);

[jvoung (off chromium), 2014/07/23 16:10:17]

similar

[Karl, 2014/07/23 20:22:21]

Done.

+    case Instruction::AShr:
+      Op = Ice::InstArithmetic::Ashr;
+      return checkIfIntArithmeticOp(Op, Ty);
+    case Instruction::And:
+      Op = Ice::InstArithmetic::And;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::Or:
+      Op = Ice::InstArithmetic::Or;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    case Instruction::Xor:
+      Op = Ice::InstArithmetic::Xor;
+      return checkIfIntOrIntVectorArithOp(Op, Ty);
+    }
+  }
+};
+
+FunctionParser::~FunctionParser() {
+  if (getTranslator().getFlags().SubzeroTimingEnabled) {
+    errs() << "[Subzero timing] Convert function "
+           << Func->getFunctionName() << ": " << TConvert.getElapsedSec()
+           << " sec\n";
+  }
+  // Before translating, check for blocks without instructions, and
+  // insert unreachable. This shouldn't happen, but be safe.
+  unsigned Index = 0;
+  for (std::vector<Ice::CfgNode*>::iterator
+           Iter = Nodes.begin(), IterEnd = Nodes.end();
+       Iter != IterEnd; ++Iter, ++Index) {
+    Ice::CfgNode *Node = *Iter;
+    if (Node->getInsts().size() == 0) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Basic block " << Index << " contains no instructions";
+      Error(StrBuf.str());
+      // TODO(kschimpf) Remove error recovery once implementation complete.
+      Node->appendInst(Ice::InstUnreachable::create(Func));
+    }
+  }
+  getTranslator().translateFcn(Func);
+}
+
+void FunctionParser::ProcessRecord() {
+  const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+  if (InstIsTerminating) {
+    InstIsTerminating = false;
+    CurrentNode = GetBasicBlock(++CurrentBbIndex);
+  }
+  Ice::Inst *Inst = NULL;
+  switch (Record.GetCode()) {
+  case naclbitc::FUNC_CODE_DECLAREBLOCKS: {
+    // DECLAREBLOCKS: [n]
+    if (checkRecordSize(1, "function block count"))
+      break;
+    if (Nodes.size() != 1) {
+      Error("Duplicate function block count record");
+      return;
+    }
+    uint32_t NumBbs = Values[0];
+    if (NumBbs == 0) {
+      Error("Functions must contain at least one basic block.");
+      // TODO(kschimpf) Remove error recovery once implementation complete.
+      NumBbs = 1;
+    }
+    // Install the basic blocks, skipping bb0 which was created in the
+    // constructor.
+    for (size_t i = 1; i < NumBbs; ++i)
+      InstallNextBasicBlock();
+    break;
+  }
+  case naclbitc::FUNC_CODE_INST_BINOP: {
+    // BINOP: [opval, opval, opcode]
+    if (checkRecordSize(3, "function block binop"))
+      break;
+    Ice::Operand *Op1 = getOperand(convertRelativeToAbsIndex(Values[0]));
+    Ice::Operand *Op2 = getOperand(convertRelativeToAbsIndex(Values[1]));
+    Ice::Type Type1 = Op1->getType();
+    Ice::Type Type2 = Op2->getType();
+    if (Type1 != Type2) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Binop argument types differ: " << Type1 << " and " << Type2;
+      Error(StrBuf.str());
+      // TODO(kschimpf) Remove error recovery once implementation complete.
+      Op2 = Op1;
+    }
+
+    Ice::InstArithmetic::OpKind Opcode;
+    if (convertBinopOpcode(Values[2], Type1, Opcode))
+      break;
+    Ice::Variable *Dest = NextInstVar(Type1);
+    Inst = Ice::InstArithmetic::create(Func, Opcode, Dest, Op1, Op2);
+    break;
+  }
+  case naclbitc::FUNC_CODE_INST_RET: {
+    // RET: [opval?]
+    InstIsTerminating = true;
+    if (checkRecordSizeInRange(0, 1, "function block ret"))
+      break;
+    if (Values.size() == 0) {
+      Inst = Ice::InstRet::create(Func);
+    } else {
+      Inst = Ice::InstRet::create(
+          Func, getOperand(convertRelativeToAbsIndex(Values[0])));
+    }
+    break;
+  }
+  default:
+    // Generate error message!
+    BlockParserBaseClass::ProcessRecord();
+    break;
+  }
+  if (Inst)
+    CurrentNode->appendInst(Inst);
+}
+
 /// Parses the module block in the bitcode file.
 class ModuleParser : public BlockParserBaseClass {
 public:
@@ -716,9 +1212,8 @@ bool ModuleParser::ParseBlock(unsigned BlockID) LLVM_OVERRIDE {
     return Parser.ParseThisBlock();
   }
   case naclbitc::FUNCTION_BLOCK_ID: {
-    Error("Function block parser not yet implemented, skipping");
-    SkipBlock();
-    return false;
+    FunctionParser Parser(BlockID, this);
+    return Parser.ParseThisBlock();
   }
   default:
     return BlockParserBaseClass::ParseBlock(BlockID);
@@ -788,13 +1283,7 @@ void ModuleParser::ProcessRecord() {
 bool TopLevelParser::ParseBlock(unsigned BlockID) {
   if (BlockID == naclbitc::MODULE_BLOCK_ID) {
     ModuleParser Parser(BlockID, this);
-    bool ReturnValue = Parser.ParseThisBlock();
-    // TODO(kschimpf): Remove once translating function blocks.
-    errs() << "Global addresses:\n";
-    for (size_t i = 0; i < ValueIDValues.size(); ++i) {
-      errs() << "[" << i << "]: " << *ValueIDValues[i] << "\n";
-    }
-    return ReturnValue;
+    return Parser.ParseThisBlock();
   }
   // Generate error message by using default block implementation.
   BlockParserBaseClass Parser(BlockID, this);
@@ -836,8 +1325,8 @@ void PNaClTranslator::translate(const std::string &IRFilename) {
   NaClBitstreamReader InputStreamFile(BufPtr, EndBufPtr);
   NaClBitstreamCursor InputStream(InputStreamFile);
 
-  TopLevelParser Parser(MemBuf->getBufferIdentifier(), Header, InputStream,
-                        ErrorStatus);
+  TopLevelParser Parser(*this, MemBuf->getBufferIdentifier(), Header,
+                        InputStream, ErrorStatus);
   int TopLevelBlocks = 0;
   while (!InputStream.AtEndOfStream()) {
     if (Parser.Parse()) {