Add switch instruction to Subzero bitcode reader.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index d63232f2be9ff5a29f5326cddd71becf019fc7c6..afc35be7617e2d948be480975a08ec377d6ab3df 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -1007,7 +1007,7 @@ private:
 
     // Must be forward reference, expand vector to accommodate.
     if (LocalIndex >= LocalOperands.size())
-      LocalOperands.resize(LocalIndex+1);
+      LocalOperands.resize(LocalIndex + 1);
 
     // If element not defined, set it.
     Ice::Operand *OldOp = LocalOperands[LocalIndex];
@@ -1023,8 +1023,8 @@ private:
     // Error has occurred.
     std::string Buffer;
     raw_string_ostream StrBuf(Buffer);
-    StrBuf << "Multiple definitions for index " << Index
-           << ": " << *Op << " and " << *OldOp;
+    StrBuf << "Multiple definitions for index " << Index << ": " << *Op
+           << " and " << *OldOp;
     Error(StrBuf.str());
     // TODO(kschimpf) Remove error recovery once implementation complete.
     LocalOperands[LocalIndex] = Op;
@@ -1037,9 +1037,7 @@ private:
   }
 
   // Returns the absolute index of the next value generating instruction.
-  uint32_t getNextInstIndex() const {
-    return NextLocalInstIndex;
-  }
+  uint32_t getNextInstIndex() const { return NextLocalInstIndex; }
 
   // Generates type error message for binary operator Op
   // operating on Type OpTy.
@@ -1682,6 +1680,70 @@ void FunctionParser::ProcessRecord() {
     InstIsTerminating = true;
     break;
   }
+  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.
+    if (!isValidRecordSizeAtLeast(4, "function block switch"))
+      return;
+    Ice::Type CondTy =
+        Context->convertToIceType(Context->getTypeByID(Values[0]));

[Jim Stichnoth, 2014/09/17 20:18:20]

Consider defining "unsigned ValIndex = 0;" up here, then using Values[ValIndex]
here, and using Values[++ValIndex] where [1], [2], and [3] are used below.

(Comment continued on line 1721.)

[Karl, 2014/09/17 22:07:19]

To try and make things clearer, I renamed ValIndex to ValCaseIndex, since it
represents the starting location of a case in the list of values. I then made
all accessors into Values (for a case) be relative
to this value.

+    if (!Ice::isScalarIntegerType(CondTy) || CondTy == Ice::IceType_i64) {

[Jim Stichnoth, 2014/09/17 20:18:20]

You can have a switch on an i64 type.

[Karl, 2014/09/17 22:07:18]

Done.

+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Case condition must be non-wide integer. Found: " << CondTy;
+      Error(StrBuf.str());
+      return;
+    }
+    Ice::SizeT BitWidth = Ice::getScalarIntBitWidth(CondTy);
+    Ice::Operand *Cond = getRelativeOperand(Values[1], BaseIndex);
+    if (CondTy != Cond->getType()) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Case condition expects type " << CondTy
+             << ". Found: " << Cond->getType();
+      Error(StrBuf.str());
+      return;
+    }
+    Ice::CfgNode *DefaultLabel = getBranchBasicBlock(Values[2]);
+    unsigned NumCases = Values[3];
+    if (!isValidRecordSize(4 + NumCases * 4, "Function block switch"))
+      return;
+    Ice::InstSwitch *Switch =
+        Ice::InstSwitch::create(Func, NumCases, Cond, DefaultLabel);
+    unsigned ValIndex = 4;
+    for (unsigned CaseIndex = 0; CaseIndex < NumCases; ++CaseIndex) {
+      if (Values[ValIndex] != 1) {

[Jim Stichnoth, 2014/09/17 20:18:20]

(continuing comment from line 1695)

ValIndex can then be changed to ++ValIndex here...

(comment continued on line 1741)

[Karl, 2014/09/17 22:07:19]

See comment above.

+        std::string Buffer;
+        raw_string_ostream StrBuf(Buffer);
+        StrBuf << "1 is expected for Values[" << ValIndex
+               << "] in switch record. Found: " << Values[ValIndex];
+        Error(StrBuf.str());
+        return;
+      }
+      if (Values[++ValIndex] != 1) {

[Jim Stichnoth, 2014/09/17 20:18:20]

Can this error test be combined with above?  E.g.

if (Values[ValIndex] != 1 || Values[++ValIndex] != 1) {

since the error message is the same.

[Karl, 2014/09/17 22:07:19]

Done.

+        std::string Buffer;
+        raw_string_ostream StrBuf(Buffer);
+        StrBuf << "1 is expected for Values[" << ValIndex
+               << "] in switch record. Found: " << Values[ValIndex];
+        Error(StrBuf.str());
+        return;
+      }
+      APInt Value(BitWidth, NaClDecodeSignRotatedValue(Values[++ValIndex]),
+                  true);
+      Ice::CfgNode *Label = getBranchBasicBlock(Values[++ValIndex]);
+      Switch->addBranch(CaseIndex, Value.getSExtValue(), Label);
+      ++ValIndex;

[Jim Stichnoth, 2014/09/17 20:18:20]

(continuing comment from line 1721)

and this ++ValIndex can be removed.

[Karl, 2014/09/17 22:07:19]

Removed.

+    }
+    CurrentNode->appendInst(Switch);
+    InstIsTerminating = true;
+    break;
+  }
   case naclbitc::FUNC_CODE_INST_UNREACHABLE: {
     // UNREACHABLE: []
     if (!isValidRecordSize(0, "function block unreachable"))
@@ -1781,11 +1843,10 @@ 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->convertToIceType(
+                              Context->getTypeByID(Values[1]))));
     break;
   }
-  case naclbitc::FUNC_CODE_INST_SWITCH:
   case naclbitc::FUNC_CODE_INST_CALL:
   case naclbitc::FUNC_CODE_INST_CALL_INDIRECT:
   default: