Remove using LLVM tools to check correctness of cast operation.

Remove using LLVM tools to check correctness of cast operation.

Removes cast instruction checks (in PNaClTranslator.cpp) that used
LLVM utilities to use locally defined methods instead. Remove the need
to call naclbitc::DecodeCastOpcode and CastInst::castIsValid.

Also removes two more calls to convertToLLVMType.

BUG= None
R=jvoung@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=bf170370805150026104a9e42e07fb17b0971ad7

[Karl, 2014-12-10 22:14:22]

kschimpf@google.com changed reviewers:
+ jvoung@chromium.org, stichnot@chromium.org

[Karl, 2014-12-12 21:20:16]

Ping?

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

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1609: void simplifyOutCommonVectorType(Ice::Type &Type1,
Ice::Type &Type2) const {
All of these new functions, up to and including isBitcastCastValid(), can be
made static.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1632: /// Returns true iff a floating type truncation
form SoruceType to TargetType
SourceType

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1638: return SourceType == Ice::IceType_f64 and
TargetType == Ice::IceType_f32;
"and"?  wow.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1647: /// Returns true iff a floating type extension
form SourceType to TargetType
from

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1653: /// Returns true if a floating type SourceType to
integer type TargetType
Use "iff" for consistency?

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

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1655: bool isFPToICastValid(Ice::Type SourceType,
Ice::Type TargetType) const {
name it isFloatToIntCastValid for consistency?

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1663: if (typeNumElements(SourceType) !=
typeNumElements(TargetType))
Might be better to just use:
  return typeNumElements(SourceType) == typeNumElements(TargetType);

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1669: /// Returns true iff an integer type SourceType to
floating type TargetType
add "cast" to the comment?

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1671: bool isIToFPCastValid(Ice::Type SourceType,
Ice::Type TargetType) const {
suggest isIntToFloatCastValid

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1687: bool isBitcastCastValid(Ice::Type SourceType,
Ice::Type TargetType) const {
maybe just call it isBitcastValid()?

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1693: /// corresponding instruction cast opcode. Also
generates error message
generates *an* error message

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1694: /// if function returns false.
what function?

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

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1628: return Ice::getScalarIntBitWidth(SourceType) >
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:19]

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1609: void simplifyOutCommonVectorType(Ice::Type &Type1,
Ice::Type &Type2) const {
On 2014/12/12 22:47:54, stichnot wrote:
> All of these new functions, up to and including isBitcastCastValid(), can be
> made static.

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1628: return Ice::getScalarIntBitWidth(SourceType) >
On 2014/12/12 23:46:45, jvoung wrote:
> 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?

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

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1632: /// Returns true iff a floating type truncation
form SoruceType to TargetType
On 2014/12/12 22:47:54, stichnot wrote:
> SourceType

Done.

Also fixed "form" -> "from".

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1638: return SourceType == Ice::IceType_f64 and
TargetType == Ice::IceType_f32;
On 2014/12/12 22:47:54, stichnot wrote:
> "and"?  wow.

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1647: /// Returns true iff a floating type extension
form SourceType to TargetType
On 2014/12/12 22:47:54, stichnot wrote:
> from

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1653: /// Returns true if a floating type SourceType to
integer type TargetType
On 2014/12/12 22:47:54, stichnot wrote:
> Use "iff" for consistency?
> 
> Also, I think the word "cast" is missing from the comment.

Done and Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1655: bool isFPToICastValid(Ice::Type SourceType,
Ice::Type TargetType) const {
On 2014/12/12 22:47:54, stichnot wrote:
> name it isFloatToIntCastValid for consistency?

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1663: if (typeNumElements(SourceType) !=
typeNumElements(TargetType))
On 2014/12/12 22:47:54, stichnot wrote:
> Might be better to just use:
>   return typeNumElements(SourceType) == typeNumElements(TargetType);

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1669: /// Returns true iff an integer type SourceType to
floating type TargetType
On 2014/12/12 22:47:55, stichnot wrote:
> add "cast" to the comment?

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1671: bool isIToFPCastValid(Ice::Type SourceType,
Ice::Type TargetType) const {
On 2014/12/12 22:47:55, stichnot wrote:
> suggest isIntToFloatCastValid

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1687: bool isBitcastCastValid(Ice::Type SourceType,
Ice::Type TargetType) const {
On 2014/12/12 22:47:54, stichnot wrote:
> maybe just call it isBitcastValid()?

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1693: /// corresponding instruction cast opcode. Also
generates error message
On 2014/12/12 22:47:54, stichnot wrote:
> generates *an* error message

Done.

https://codereview.chromium.org/794823002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1694: /// if function returns false.
On 2014/12/12 22:47:55, stichnot wrote:
> what function?

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

[jvoung (off chromium), 2014-12-15 18:11:44]

lgtm

[Jim Stichnoth, 2014-12-15 18:12:25]

also lgtm

[Karl, 2014-12-15 18:16:43]

Committed patchset #5 (id:80001) manually as
bf170370805150026104a9e42e07fb17b0971ad7 (presubmit successful).