Remove using LLVM tools to check correctness of cast operation.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 124ed894f34fd72da298a1c43b253227479e7179..23dcdad3f57de6aa1f60e5d2fb652c4b10a7bd2d 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -1604,46 +1604,157 @@ private:
     }
   }
 
-  /// Converts an LLVM cast opcode LLVMCastOp to the corresponding Ice
-  /// cast opcode and assigns to CastKind. Returns true if successful,
-  /// false otherwise.
-  bool convertLLVMCastOpToIceOp(Instruction::CastOps LLVMCastOp,
-                                Ice::InstCast::OpKind &CastKind) const {
-    switch (LLVMCastOp) {
-    case Instruction::ZExt:
+  /// Simplifies out vector types from Type1 and Type2, if both are vectors
+  /// of the same size.
+  void simplifyOutCommonVectorType(Ice::Type &Type1, Ice::Type &Type2) const {

[Jim Stichnoth, 2014/12/12 22:47:54]

All of these new functions, up to and including isBitcastCastValid(), can be
made static.

[Karl, 2014/12/15 17:40:18]

Done.

+    bool IsType1Vector = isVectorType(Type1);
+    bool IsType2Vector = isVectorType(Type2);
+    if (IsType1Vector != IsType2Vector)
+      return;
+    if (IsType1Vector) {
+      if (typeNumElements(Type1) != typeNumElements(Type2))
+        return;
+      Type1 = typeElementType(Type1);
+      Type2 = typeElementType(Type2);
+    }
+  }
+
+  /// Returns true iff an integer truncation from SourceType to TargetType is
+  /// valid.
+  bool isIntTruncCastValid(Ice::Type SourceType, Ice::Type TargetType) const {
+    if (!(Ice::isIntegerType(SourceType) && Ice::isIntegerType(TargetType)))
+      return false;
+    simplifyOutCommonVectorType(SourceType, TargetType);
+    return Ice::getScalarIntBitWidth(SourceType) >

[jvoung (off chromium), 2014/12/12 23:46:45]

nit: One of the getScalarIntBitWidth is prefixed with "Ice::", but the other is
not. Make consistent?

It's a little non-obvious what happens if one is a vector int type, and the
other is a scalar int type. 

simplifyOutCommonVectorType() will return early if there is such a mismatch,
leaving the vector a vector, and the scalar a scalar.

Then there is an assert() that getScalarIntBitWidth(x) is given an x that is a
scalar. However, if asserts are turned off then it would just continue running
and call typeWidthInBytes * CHAR_BIT.

I guess I'm wondering if simplifyOutCommonVectorType() should be more strict
about the two types being both vectors or both scalars?

[Karl, 2014/12/15 17:40:18]

Good point. When I factored this out, I forgot to have it return success only if
both were vector, or both were scalar. Fixing.

+           getScalarIntBitWidth(TargetType);
+  }
+
+  /// Returns true iff a floating type truncation form SoruceType to TargetType

[Jim Stichnoth, 2014/12/12 22:47:54]

SourceType

[Karl, 2014/12/15 17:40:18]

Done.

Also fixed "form" -> "from".

+  /// is valid.
+  bool isFloatTruncCastValid(Ice::Type SourceType, Ice::Type TargetType) const {
+    if (!(Ice::isFloatingType(SourceType) && Ice::isFloatingType(TargetType)))
+      return false;
+    simplifyOutCommonVectorType(SourceType, TargetType);
+    return SourceType == Ice::IceType_f64 and TargetType == Ice::IceType_f32;

[Jim Stichnoth, 2014/12/12 22:47:54]

"and"?  wow.

[Karl, 2014/12/15 17:40:18]

Done.

+  }
+
+  /// Returns true iff an integer extension from SourceType to TargetType is
+  /// valid.
+  bool isIntExtCastValid(Ice::Type SourceType, Ice::Type TargetType) const {
+    return isIntTruncCastValid(TargetType, SourceType);
+  }
+
+  /// Returns true iff a floating type extension form SourceType to TargetType

[Jim Stichnoth, 2014/12/12 22:47:54]

from

[Karl, 2014/12/15 17:40:18]

Done.

+  /// is valid.
+  bool isFloatExtCastValid(Ice::Type SourceType, Ice::Type TargetType) const {
+    return isFloatTruncCastValid(TargetType, SourceType);
+  }
+
+  /// Returns true if a floating type SourceType to integer type TargetType

[Jim Stichnoth, 2014/12/12 22:47:54]

Use "iff" for consistency?

Also, I think the word "cast" is missing from the comment.

[Karl, 2014/12/15 17:40:18]

Done and Done.

+  /// is valid.
+  bool isFPToICastValid(Ice::Type SourceType, Ice::Type TargetType) const {

[Jim Stichnoth, 2014/12/12 22:47:54]

name it isFloatToIntCastValid for consistency?

[Karl, 2014/12/15 17:40:18]

Done.

+    if (!(Ice::isFloatingType(SourceType) && Ice::isIntegerType(TargetType)))
+      return false;
+    bool IsSourceVector = isVectorType(SourceType);
+    bool IsTargetVector = isVectorType(TargetType);
+    if (IsSourceVector != IsTargetVector)
+      return false;
+    if (IsSourceVector) {
+      if (typeNumElements(SourceType) != typeNumElements(TargetType))

[Jim Stichnoth, 2014/12/12 22:47:54]

Might be better to just use:
  return typeNumElements(SourceType) == typeNumElements(TargetType);

[Karl, 2014/12/15 17:40:18]

Done.

+        return false;
+    }
+    return true;
+  }
+
+  /// Returns true iff an integer type SourceType to floating type TargetType

[Jim Stichnoth, 2014/12/12 22:47:55]

add "cast" to the comment?

[Karl, 2014/12/15 17:40:18]

Done.

+  /// is valid.
+  bool isIToFPCastValid(Ice::Type SourceType, Ice::Type TargetType) const {

[Jim Stichnoth, 2014/12/12 22:47:55]

suggest isIntToFloatCastValid

[Karl, 2014/12/15 17:40:18]

Done.

+    return isFPToICastValid(TargetType, SourceType);
+  }
+
+  /// Returns the number of bits used to model type Ty when defining the
+  /// bitcast instruction.
+  Ice::SizeT bitcastSizeInBits(Ice::Type Ty) const {
+    if (Ice::isVectorType(Ty))
+      return Ice::typeNumElements(Ty) *
+             bitcastSizeInBits(Ice::typeElementType(Ty));
+    if (Ty == Ice::IceType_i1)
+      return 1;
+    return Ice::typeWidthInBytes(Ty) * CHAR_BIT;
+  }
+
+  /// Returns true if a bitcast from SourceType to TargetType is allowed.
+  bool isBitcastCastValid(Ice::Type SourceType, Ice::Type TargetType) const {

[Jim Stichnoth, 2014/12/12 22:47:54]

maybe just call it isBitcastValid()?

[Karl, 2014/12/15 17:40:18]

Done.

+    return bitcastSizeInBits(SourceType) == bitcastSizeInBits(TargetType);
+  }
+
+  /// Returns true if the NaCl bitcode Opcode is a valid cast opcode
+  /// for converting SourceType to TargetType. Updates CastKind to the
+  /// corresponding instruction cast opcode. Also generates error message

[Jim Stichnoth, 2014/12/12 22:47:54]

generates *an* error message

[Karl, 2014/12/15 17:40:19]

Done.

+  /// if function returns false.

[Jim Stichnoth, 2014/12/12 22:47:55]

what function?

[Karl, 2014/12/15 17:40:18]

Added "this" and replaced "if" with "when".

+  bool convertCastOpToIceOp(uint64_t Opcode, Ice::Type SourceType,
+                            Ice::Type TargetType,
+                            Ice::InstCast::OpKind &CastKind) {
+    bool Result;
+    switch (Opcode) {
+    default: {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Cast opcode " << Opcode << " not understood.\n";
+      Error(StrBuf.str());
+      // TODO(kschimpf) Remove error recovery once implementation complete.
+      CastKind = Ice::InstCast::Bitcast;
+      return false;
+    }
+    case naclbitc::CAST_TRUNC:
+      CastKind = Ice::InstCast::Trunc;
+      Result = isIntTruncCastValid(SourceType, TargetType);
+      break;
+    case naclbitc::CAST_ZEXT:
       CastKind = Ice::InstCast::Zext;
+      Result = isIntExtCastValid(SourceType, TargetType);
       break;
-    case Instruction::SExt:
+    case naclbitc::CAST_SEXT:
       CastKind = Ice::InstCast::Sext;
+      Result = isIntExtCastValid(SourceType, TargetType);
       break;
-    case Instruction::Trunc:
-      CastKind = Ice::InstCast::Trunc;
-      break;
-    case Instruction::FPTrunc:
-      CastKind = Ice::InstCast::Fptrunc;
-      break;
-    case Instruction::FPExt:
-      CastKind = Ice::InstCast::Fpext;
+    case naclbitc::CAST_FPTOUI:
+      CastKind = Ice::InstCast::Fptoui;
+      Result = isFPToICastValid(SourceType, TargetType);
       break;
-    case Instruction::FPToSI:
+    case naclbitc::CAST_FPTOSI:
       CastKind = Ice::InstCast::Fptosi;
+      Result = isFPToICastValid(SourceType, TargetType);
       break;
-    case Instruction::FPToUI:
-      CastKind = Ice::InstCast::Fptoui;
+    case naclbitc::CAST_UITOFP:
+      CastKind = Ice::InstCast::Uitofp;
+      Result = isIToFPCastValid(SourceType, TargetType);
       break;
-    case Instruction::SIToFP:
+    case naclbitc::CAST_SITOFP:
       CastKind = Ice::InstCast::Sitofp;
+      Result = isIToFPCastValid(SourceType, TargetType);
       break;
-    case Instruction::UIToFP:
-      CastKind = Ice::InstCast::Uitofp;
+    case naclbitc::CAST_FPTRUNC:
+      CastKind = Ice::InstCast::Fptrunc;
+      Result = isFloatTruncCastValid(SourceType, TargetType);
+      break;
+    case naclbitc::CAST_FPEXT:
+      CastKind = Ice::InstCast::Fpext;
+      Result = isFloatExtCastValid(SourceType, TargetType);
       break;
-    case Instruction::BitCast:
+    case naclbitc::CAST_BITCAST:
       CastKind = Ice::InstCast::Bitcast;
+      Result = isBitcastCastValid(SourceType, TargetType);
       break;
-    default:
-      return false;
     }
-    return true;
+    if (!Result) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Illegal cast: " << Ice::InstCast::getCastName(CastKind) << " "
+             << SourceType << " to " << TargetType;
+      Error(StrBuf.str());
+    }
+    return Result;
   }
 
   // Converts PNaCl bitcode Icmp operator to corresponding ICE op.
@@ -1876,30 +1987,13 @@ void FunctionParser::ProcessRecord() {
       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;
-    }
     if (isIRGenerationDisabled()) {
       assert(Src == nullptr);
       setNextLocalInstIndex(nullptr);
       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());
+    if (!convertCastOpToIceOp(Values[2], Src->getType(), CastType, CastKind)) {
       appendErrorInstruction(CastType);
       return;
     }