Fix insert/extract element vector operations to check index is literal

src/PNaClTranslator.cpp

 //===- subzero/src/PNaClTranslator.cpp - ICE from bitcode -----------------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the PNaCl bitcode file to Ice, to machine code
@@ skipping 1298 matching lines @@
                                           Context->convertToLLVMType(Ty)))
       return true;
     std::string Buffer;
     raw_string_ostream StrBuf(Buffer);
     StrBuf << InstructionName << " " << Ty << "*: not allowed for alignment "
            << Alignment;
     Error(StrBuf.str());
     return false;
   }
 
+  // Types of errors that can occur for insertelement and extractelement
+  // instructions.
+  enum VectorIndexCheckValue {
+    VectorIndexValid,
+    VectorIndexOnNonVector,
+    VectorIndexNotI32,
+    VectorIndexNotInRange,
+    VectorIndexNotConstant
+  };
+
+  void DumpVectorIndexCheckValue(raw_ostream &Stream,

[Jim Stichnoth, 2014/10/28 22:25:54]

dumpVectorIndexCheckValue

[Karl, 2014/10/29 17:00:58]

Done.

+                                 VectorIndexCheckValue Value) {

[Jim Stichnoth, 2014/10/28 22:25:54]

Can this method be const?  Or static?

[Karl, 2014/10/29 17:00:57]

Done.

+    switch (Value) {
+    default:
+      report_fatal_error("Unknown VectorIndexCheckValue");
+      break;
+    case VectorIndexValid:
+      Stream << "Valid vector index";
+      break;
+    case VectorIndexOnNonVector:
+      Stream << "Vector index on non vector";
+      break;
+    case VectorIndexNotI32:
+      Stream << "Vector index not of type " << Ice::IceType_i32;
+      break;
+    case VectorIndexNotInRange:
+      Stream << "Vector index not in range of vector";
+      break;
+    case VectorIndexNotConstant:
+      Stream << "Vector index not integer constant";
+      break;
+    }
+  }
+
+  // Returns whether the given vector index (for insertelement and
+  // extractelement instructions) is valid.
+  VectorIndexCheckValue validateVectorIndex(const Ice::Operand *Vec,

[Jim Stichnoth, 2014/10/28 22:25:54]

Const or static?

[Karl, 2014/10/29 17:00:58]

Done.

+                                            const Ice::Operand *Index) {
+    Ice::Type VecType = Vec->getType();
+    if (!Ice::isVectorType(VecType))
+      return VectorIndexOnNonVector;
+    if (Index->getType() != Ice::IceType_i32)
+      return VectorIndexNotI32;

[Jim Stichnoth, 2014/10/28 22:25:54]

This is kind of nitpicky, but which error should be returned if the vector index
is a variable of type i16 - NotI32 or NotConstant?  From a user's point of view,
I think NotConstant would be better.

And to continue on this nitpickiness path, I would rearrange the order in
VectorIndexCheckValue, dumpVectorIndexCheck(), and validateVectorIndex() to be:
  NotVector
  NotConstant
  NotI32
  NotInRange
  Valid
I think that would especially make this function look cleaner - flat instead of
an if nest.

[Karl, 2014/10/29 17:00:58]

I agree this is better. However, I would interchange NotI32 and NotInRange (you
are further away if the index is out of range, than the type).

+    if (const auto *C = dyn_cast<Ice::ConstantInteger32>(Index)) {
+      if (C->getValue() <= typeNumElements(VecType)) {

[Jim Stichnoth, 2014/10/28 22:25:54]

This should be '<', not '<=', right?

[Karl, 2014/10/29 17:00:57]

Yes.

+        return VectorIndexValid;
+      } else {
+        return VectorIndexNotInRange;
+      }
+    } else {
+      return VectorIndexNotConstant;
+    }
+  }
+
   // Reports that the given binary Opcode, for the given type Ty,
   // is not understood.
   void ReportInvalidBinopOpcode(unsigned Opcode, Ice::Type Ty);
 
   // Returns true if the Str begins with Prefix.
   bool isStringPrefix(Ice::IceString &Str, Ice::IceString &Prefix) {
     const size_t PrefixSize = Prefix.size();
     if (Str.size() < PrefixSize)
       return false;
     for (size_t i = 0; i < PrefixSize; ++i) {
@@ skipping 212 matching lines @@
     case naclbitc::FCMP_UNE:
       Cond = Ice::InstFcmp::Une;
       return true;
     case naclbitc::FCMP_TRUE:
       Cond = Ice::InstFcmp::True;
       return true;
     default:
       return false;
     }
   }
+
+  // Creates an error instruction, generating a value of type Ty, and
+  // adds a placeholder so that instruction indices line up. If Ty is
+  // void, no placeholder is needed and the error instruction is not
+  // appended.

[Jim Stichnoth, 2014/10/28 22:25:54]

Maybe add a note about why Ty would be void - an invalid call that has no dest
because the call is void.

[Karl, 2014/10/29 17:00:57]

Done.

+  // TODO(kschimpf) Remove error recovery once implementation complete.
+  void appendErrorInstruction(Ice::Type Ty) {

[Jim Stichnoth, 2014/10/28 22:25:54]

const?

[Karl, 2014/10/29 17:00:57]

Not possible, modifies state when calling getNextInstVar.

+    // Note: we don't worry about downstream translation errors because
+    // the function will not be translated if any errors occur.
+    if (Ty == Ice::IceType_void)
+      return;
+    Ice::Variable *Var = getNextInstVar(Ty);
+    CurrentNode->appendInst(Ice::InstAssign::create(Func, Var, Var));
+  }
 };
 
 void FunctionParser::ReportInvalidBinopOpcode(unsigned Opcode, Ice::Type Ty) {
   std::string Buffer;
   raw_string_ostream StrBuf(Buffer);
   StrBuf << "Binary opcode " << Opcode << "not understood for type " << Ty;
   Error(StrBuf.str());
 }
 
 void FunctionParser::ExitBlock() {
@@ skipping 54 matching lines @@
     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;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_BINOP: {
     // BINOP: [opval, opval, opcode]
     if (!isValidRecordSize(3, "function block binop"))
       return;
     Ice::Operand *Op1 = getRelativeOperand(Values[0], BaseIndex);
     Ice::Operand *Op2 = getRelativeOperand(Values[1], BaseIndex);
     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;
+      appendErrorInstruction(Type1);
+      return;
     }
 
     Ice::InstArithmetic::OpKind Opcode;
     if (!convertBinopOpcode(Values[2], Type1, Opcode))
       return;
     CurrentNode->appendInst(Ice::InstArithmetic::create(
         Func, Opcode, getNextInstVar(Type1), Op1, Op2));
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_CAST: {
     // CAST: [opval, destty, castopc]
     if (!isValidRecordSize(3, "function block cast"))
       return;
     Ice::Operand *Src = getRelativeOperand(Values[0], BaseIndex);
     Ice::Type CastType = Context->getSimpleTypeByID(Values[1]);
     Instruction::CastOps LLVMCastOp;
     Ice::InstCast::OpKind CastKind;
     if (!naclbitc::DecodeCastOpcode(Values[2], LLVMCastOp) ||
         !convertLLVMCastOpToIceOp(LLVMCastOp, CastKind)) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Cast opcode not understood: " << Values[2];
       Error(StrBuf.str());
+      appendErrorInstruction(CastType);
       return;
     }
     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;
       Error(StrBuf.str());
+      appendErrorInstruction(CastType);
       return;
     }
     CurrentNode->appendInst(
         Ice::InstCast::create(Func, CastKind, getNextInstVar(CastType), Src));
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_VSELECT: {
     // VSELECT: [opval, opval, pred]
     Ice::Operand *ThenVal = getRelativeOperand(Values[0], BaseIndex);
     Ice::Type ThenType = ThenVal->getType();
     Ice::Operand *ElseVal = getRelativeOperand(Values[1], BaseIndex);
     Ice::Type ElseType = ElseVal->getType();
     if (ThenType != ElseType) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Select operands not same type. Found " << ThenType << " and "
              << ElseType;
       Error(StrBuf.str());
+      appendErrorInstruction(ThenType);
       return;
     }
     Ice::Operand *CondVal = getRelativeOperand(Values[2], BaseIndex);
     Ice::Type CondType = CondVal->getType();
     if (isVectorType(CondType)) {
       if (!isVectorType(ThenType) ||
           typeElementType(CondType) != Ice::IceType_i1 ||
           typeNumElements(ThenType) != typeNumElements(CondType)) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
         StrBuf << "Select condition type " << CondType
                << " not allowed for values of type " << ThenType;
         Error(StrBuf.str());
+        appendErrorInstruction(ThenType);
         return;
       }
     } else if (CondVal->getType() != Ice::IceType_i1) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Select condition " << CondVal << " not type i1. Found: "
              << CondVal->getType();
       Error(StrBuf.str());
+      appendErrorInstruction(ThenType);
       return;
     }
     CurrentNode->appendInst(Ice::InstSelect::create(
         Func, getNextInstVar(ThenType), CondVal, ThenVal, ElseVal));
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_EXTRACTELT: {
     // EXTRACTELT: [opval, opval]
     if (!isValidRecordSize(2, "function block extract element"))
       return;
     Ice::Operand *Vec = getRelativeOperand(Values[0], BaseIndex);
     Ice::Type VecType = Vec->getType();
-    if (!Ice::isVectorType(VecType)) {
+    Ice::Operand *Index = getRelativeOperand(Values[1], BaseIndex);
+    VectorIndexCheckValue IndexCheckValue = validateVectorIndex(Vec, Index);
+    if (IndexCheckValue != VectorIndexValid) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Extractelement not on vector. Found: " << *Vec;
+      DumpVectorIndexCheckValue(StrBuf, IndexCheckValue);
+      StrBuf << ": extractelement " << VecType << " " << *Vec << ", "
+             << Index->getType() << " " << *Index;
       Error(StrBuf.str());
+      appendErrorInstruction(VecType);
+      return;
     }
-    Ice::Operand *Index = getRelativeOperand(Values[1], BaseIndex);
-    if (Index->getType() != Ice::IceType_i32) {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Extractelement index " << *Index << " not i32. Found: "
-             << Index->getType();
-      Error(StrBuf.str());
-    }
-    // TODO(kschimpf): Restrict index to a legal constant index (once
-    // constants can be defined).
     CurrentNode->appendInst(Ice::InstExtractElement::create(
         Func, getNextInstVar(typeElementType(VecType)), Vec, Index));
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_INSERTELT: {
     // INSERTELT: [opval, opval, opval]
     if (!isValidRecordSize(3, "function block insert element"))
       return;
     Ice::Operand *Vec = getRelativeOperand(Values[0], BaseIndex);
     Ice::Type VecType = Vec->getType();
-    if (!Ice::isVectorType(VecType)) {
+    Ice::Operand *Elt = getRelativeOperand(Values[1], BaseIndex);
+    Ice::Operand *Index = getRelativeOperand(Values[2], BaseIndex);
+    VectorIndexCheckValue IndexCheckValue = validateVectorIndex(Vec, Index);
+    if (IndexCheckValue != VectorIndexValid) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Insertelement not on vector. Found: " << *Vec;
+      DumpVectorIndexCheckValue(StrBuf, IndexCheckValue);
+      StrBuf << ": insertelement " << VecType << " " << *Vec << ", "
+             << Elt->getType() << " " << *Elt << ", " << Index->getType() << " "
+             << *Index;
       Error(StrBuf.str());
+      appendErrorInstruction(Elt->getType());
+      return;
     }
-    Ice::Operand *Elt = getRelativeOperand(Values[1], BaseIndex);
-    Ice::Type EltType = Elt->getType();
-    if (EltType != typeElementType(VecType)) {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Insertelement element " << *Elt << " not type "
-             << typeElementType(VecType)
-             << ". Found: " << EltType;
-      Error(StrBuf.str());
-    }
-    Ice::Operand *Index = getRelativeOperand(Values[2], BaseIndex);
-    if (Index->getType() != Ice::IceType_i32) {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Insertelement index " << *Index << " not i32. Found: "
-             << Index->getType();
-      Error(StrBuf.str());
-    }
-    // TODO(kschimpf): Restrict index to a legal constant index (once
-    // constants can be defined).
     CurrentNode->appendInst(Ice::InstInsertElement::create(
         Func, getNextInstVar(VecType), Vec, Elt, Index));
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_CMP2: {
     // CMP2: [opval, opval, pred]
     if (!isValidRecordSize(3, "function block compare"))
       return;
     Ice::Operand *Op1 = getRelativeOperand(Values[0], BaseIndex);
     Ice::Operand *Op2 = getRelativeOperand(Values[1], BaseIndex);
     Ice::Type Op1Type = Op1->getType();
     Ice::Type Op2Type = Op2->getType();
+    Ice::Type DestType = getCompareResultType(Op1Type);
     if (Op1Type != Op2Type) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Compare argument types differ: " << Op1Type
              << " and " << Op2Type;
       Error(StrBuf.str());
-      // TODO(kschimpf) Remove error recovery once implementation complete.
+      appendErrorInstruction(DestType);
       Op2 = Op1;
     }
-    Ice::Type DestType = getCompareResultType(Op1Type);
     if (DestType == Ice::IceType_void) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Compare not defined for type " << Op1Type;
       Error(StrBuf.str());
       return;
     }
     Ice::Variable *Dest = getNextInstVar(DestType);
     if (isIntegerType(Op1Type)) {
       Ice::InstIcmp::ICond Cond;
       if (!convertNaClBitcICmpOpToIce(Values[2], Cond)) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
         StrBuf << "Compare record contains unknown integer predicate index: "
                << Values[2];
         Error(StrBuf.str());
-        // TODO(kschimpf) Remove error recovery once implementation complete.
-        Cond = Ice::InstIcmp::Eq;
+        appendErrorInstruction(DestType);
       }
       CurrentNode->appendInst(
           Ice::InstIcmp::create(Func, Cond, Dest, Op1, Op2));
     } else if (isFloatingType(Op1Type)){
       Ice::InstFcmp::FCond Cond;
       if (!convertNaClBitcFCompOpToIce(Values[2], Cond)) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
         StrBuf << "Compare record contains unknown float predicate index: "
                << Values[2];
         Error(StrBuf.str());
-        // TODO(kschimpf) Remove error recovery once implementation complete.
-        Cond = Ice::InstFcmp::False;
+        appendErrorInstruction(DestType);
       }
       CurrentNode->appendInst(
           Ice::InstFcmp::create(Func, Cond, Dest, Op1, Op2));
     } else {
       // Not sure this can happen, but be safe.
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Compare on type not understood: " << Op1Type;
       Error(StrBuf.str());
+      appendErrorInstruction(DestType);
       return;
     }
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_RET: {
     // RET: [opval?]
     if (!isValidRecordSizeInRange(0, 1, "function block ret"))
       return;
     if (Values.size() == 0) {
       CurrentNode->appendInst(Ice::InstRet::create(Func));
     } else {
       CurrentNode->appendInst(
           Ice::InstRet::create(Func, getRelativeOperand(Values[0], BaseIndex)));
     }
     InstIsTerminating = true;
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_BR: {
     if (Values.size() == 1) {
       // BR: [bb#]
       Ice::CfgNode *Block = getBranchBasicBlock(Values[0]);
       if (Block == nullptr)
         return;
       CurrentNode->appendInst(Ice::InstBr::create(Func, Block));
     } else {
       // BR: [bb#, bb#, opval]
       if (!isValidRecordSize(3, "function block branch"))
         return;
       Ice::Operand *Cond = getRelativeOperand(Values[2], BaseIndex);
       if (Cond->getType() != Ice::IceType_i1) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
         StrBuf << "Branch condition " << *Cond << " not i1. Found: "
                << Cond->getType();
         Error(StrBuf.str());
         return;
       }
       Ice::CfgNode *ThenBlock = getBranchBasicBlock(Values[0]);
       Ice::CfgNode *ElseBlock = getBranchBasicBlock(Values[1]);
       if (ThenBlock == nullptr || ElseBlock == nullptr)
         return;
       CurrentNode->appendInst(
           Ice::InstBr::create(Func, Cond, ThenBlock, ElseBlock));
     }
     InstIsTerminating = true;
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_SWITCH: {
     // SWITCH: [Condty, Cond, BbIndex, NumCases Case ...]
     // where Case = [1, 1, Value, BbIndex].
     //
     // Note: Unlike most instructions, we don't infer the type of
     // Cond, but provide it as a separate field. There are also
     // unnecesary data fields (i.e. constants 1).  These were not
     // cleaned up in PNaCl bitcode because the bitcode format was
     // already frozen when the problem was noticed.
@@ skipping 37 matching lines @@
         return;
       }
       APInt Value(BitWidth,
                   NaClDecodeSignRotatedValue(Values[ValCaseIndex + 2]),
                   true);
       Ice::CfgNode *Label = getBranchBasicBlock(Values[ValCaseIndex + 3]);
       Switch->addBranch(CaseIndex, Value.getSExtValue(), Label);
     }
     CurrentNode->appendInst(Switch);
     InstIsTerminating = true;
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_UNREACHABLE: {
     // UNREACHABLE: []
     if (!isValidRecordSize(0, "function block unreachable"))
       return;
     CurrentNode->appendInst(
         Ice::InstUnreachable::create(Func));
     InstIsTerminating = true;
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_PHI: {
     // PHI: [ty, val1, bb1, ..., valN, bbN] for n >= 2.
     if (!isValidRecordSizeAtLeast(3, "function block phi"))
       return;
+    Ice::Type Ty = Context->getSimpleTypeByID(Values[0]);
     if ((Values.size() & 0x1) == 0) {
       // Not an odd number of values.
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "function block phi record size not valid: " << Values.size();
       Error(StrBuf.str());
+      appendErrorInstruction(Ty);
       return;
     }
-    Ice::Type Ty = Context->getSimpleTypeByID(Values[0]);
     if (Ty == Ice::IceType_void) {
       Error("Phi record using type void not allowed");
       return;
     }
     Ice::Variable *Dest = getNextInstVar(Ty);
     Ice::InstPhi *Phi = Ice::InstPhi::create(Func, Values.size() >> 1, Dest);
     for (unsigned i = 1; i < Values.size(); i += 2) {
       Ice::Operand *Op =
           getRelativeOperand(NaClDecodeSignRotatedValue(Values[i]), BaseIndex);
       if (Op->getType() != Ty) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
         StrBuf << "Value " << *Op << " not type " << Ty
                << " in phi instruction. Found: " << Op->getType();
         Error(StrBuf.str());
+        appendErrorInstruction(Ty);
         return;
       }
       Phi->addArgument(Op, getBasicBlock(Values[i + 1]));
     }
     CurrentNode->appendInst(Phi);
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_ALLOCA: {
     // ALLOCA: [Size, align]
     if (!isValidRecordSize(2, "function block alloca"))
       return;
     Ice::Operand *ByteCount = getRelativeOperand(Values[0], BaseIndex);
+    Ice::Type PtrTy = Context->getIcePointerType();
     if (ByteCount->getType() != Ice::IceType_i32) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Alloca on non-i32 value. Found: " << *ByteCount;
       Error(StrBuf.str());
+      appendErrorInstruction(PtrTy);
       return;
     }
     unsigned Alignment;
     extractAlignment("Alloca", Values[1], Alignment);
-    CurrentNode->appendInst(
-        Ice::InstAlloca::create(Func, ByteCount, Alignment,
-                                getNextInstVar(Context->getIcePointerType())));
-    break;
+    CurrentNode->appendInst(Ice::InstAlloca::create(Func, ByteCount, Alignment,
+                                                    getNextInstVar(PtrTy)));
+    return;
   }
   case naclbitc::FUNC_CODE_INST_LOAD: {
     // LOAD: [address, align, ty]
     if (!isValidRecordSize(3, "function block load"))
       return;
     Ice::Operand *Address = getRelativeOperand(Values[0], BaseIndex);
-    if (!isValidPointerType(Address, "Load"))
+    Ice::Type Ty = Context->getSimpleTypeByID(Values[2]);
+    if (!isValidPointerType(Address, "Load")) {
+      appendErrorInstruction(Ty);
       return;
+    }
     unsigned Alignment;
     extractAlignment("Load", Values[1], Alignment);
-    Ice::Type Ty = Context->getSimpleTypeByID(Values[2]);
-    if (!isValidLoadStoreAlignment(Alignment, Ty, "Load"))
+    if (!isValidLoadStoreAlignment(Alignment, Ty, "Load")) {
+      appendErrorInstruction(Ty);
       return;
+    }
     CurrentNode->appendInst(
         Ice::InstLoad::create(Func, getNextInstVar(Ty), Address, Alignment));
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_STORE: {
     // STORE: [address, value, align]
     if (!isValidRecordSize(3, "function block store"))
       return;
     Ice::Operand *Address = getRelativeOperand(Values[0], BaseIndex);
     if (!isValidPointerType(Address, "Store"))
       return;
     Ice::Operand *Value = getRelativeOperand(Values[1], BaseIndex);
     unsigned Alignment;
     extractAlignment("Store", Values[2], Alignment);
     if (!isValidLoadStoreAlignment(Alignment, Value->getType(), "Store"))
       return;
     CurrentNode->appendInst(
         Ice::InstStore::create(Func, Value, Address, Alignment));
-    break;
+    return;
   }
   case naclbitc::FUNC_CODE_INST_CALL:
   case naclbitc::FUNC_CODE_INST_CALL_INDIRECT: {
     // CALL: [cc, fnid, arg0, arg1...]
     // CALL_INDIRECT: [cc, fn, returnty, args...]
     //
     // Note: The difference between CALL and CALL_INDIRECT is that
     // CALL has a reference to an explicit function declaration, while
     // the CALL_INDIRECT is just an address. For CALL, we can infer
     // the return type by looking up the type signature associated
     // with the function declaration. For CALL_INDIRECT we can only
     // infer the type signature via argument types, and the
     // corresponding return type stored in CALL_INDIRECT record.
     Ice::SizeT ParamsStartIndex = 2;
     if (Record.GetCode() == naclbitc::FUNC_CODE_INST_CALL) {
       if (!isValidRecordSizeAtLeast(2, "function block call"))
         return;
     } else {
       if (!isValidRecordSizeAtLeast(3, "function block call indirect"))
         return;
       ParamsStartIndex = 3;
     }
 
-    // Extract call information.
-    uint64_t CCInfo = Values[0];
-    CallingConv::ID CallingConv;
-    if (!naclbitc::DecodeCallingConv(CCInfo >> 1, CallingConv)) {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Function call calling convention value " << (CCInfo >> 1)
-             << " not understood.";
-      Error(StrBuf.str());
-      return;
-    }
-    bool IsTailCall = static_cast<bool>(CCInfo & 1);
-
     // Extract out the called function and its return type.
     uint32_t CalleeIndex = convertRelativeToAbsIndex(Values[1], BaseIndex);
     Ice::Operand *Callee = getOperand(CalleeIndex);
     Ice::Type ReturnType = Ice::IceType_void;
     const Ice::Intrinsics::FullIntrinsicInfo *IntrinsicInfo = nullptr;
     if (Record.GetCode() == naclbitc::FUNC_CODE_INST_CALL) {
       Ice::FunctionDeclaration *Fcn = Context->getFunctionByID(CalleeIndex);
       const Ice::FuncSigType &Signature = Fcn->getSignature();
       ReturnType = Signature.getReturnType();
 
       // Check if this direct call is to an Intrinsic (starts with "llvm.")
       static Ice::IceString LLVMPrefix("llvm.");
       Ice::IceString Name = Fcn->getName();
       if (isStringPrefix(Name, LLVMPrefix)) {
         Ice::IceString Suffix = Name.substr(LLVMPrefix.size());
         IntrinsicInfo =
             getTranslator().getContext()->getIntrinsicsInfo().find(Suffix);
         if (!IntrinsicInfo) {
           std::string Buffer;
           raw_string_ostream StrBuf(Buffer);
           StrBuf << "Invalid PNaCl intrinsic call to " << Name;
           Error(StrBuf.str());
+          appendErrorInstruction(ReturnType);
           return;
         }
       }
     } else {
       ReturnType = Context->getSimpleTypeByID(Values[2]);
     }
 
+    // Extract call information.
+    uint64_t CCInfo = Values[0];
+    CallingConv::ID CallingConv;
+    if (!naclbitc::DecodeCallingConv(CCInfo >> 1, CallingConv)) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Function call calling convention value " << (CCInfo >> 1)
+             << " not understood.";
+      Error(StrBuf.str());
+      appendErrorInstruction(ReturnType);
+      return;
+    }
+    bool IsTailCall = static_cast<bool>(CCInfo & 1);
+
     // Create the call instruction.
     Ice::Variable *Dest = (ReturnType == Ice::IceType_void)
                               ? nullptr
                               : getNextInstVar(ReturnType);
     Ice::SizeT NumParams = Values.size() - ParamsStartIndex;
     Ice::InstCall *Inst = nullptr;
     if (IntrinsicInfo) {
       Inst =
           Ice::InstIntrinsicCall::create(Func, NumParams, Dest, Callee,
                                          IntrinsicInfo->Info);
@@ skipping 50 matching lines @@
     }
 
     CurrentNode->appendInst(Inst);
     return;
   }
   case naclbitc::FUNC_CODE_INST_FORWARDTYPEREF: {
     // FORWARDTYPEREF: [opval, ty]
     if (!isValidRecordSize(2, "function block forward type ref"))
       return;
     setOperand(Values[0], createInstVar(Context->getSimpleTypeByID(Values[1])));
-    break;
+    return;
   }
   default:
     // Generate error message!
     BlockParserBaseClass::ProcessRecord();
-    break;
+    return;
   }
 }
 
 /// Parses constants within a function block.
 class ConstantsParser : public BlockParserBaseClass {
   ConstantsParser(const ConstantsParser &) = delete;
   ConstantsParser &operator=(const ConstantsParser &) = delete;
 
 public:
   ConstantsParser(unsigned BlockID, FunctionParser *FuncParser)
@@ skipping 409 matching lines @@
 
   if (TopLevelBlocks != 1) {
     errs() << IRFilename
            << ": Contains more than one module. Found: " << TopLevelBlocks
            << "\n";
     ErrorStatus = true;
   }
 }
 
 } // end of namespace Ice