Change some explicit type checks into using helper functions.

Change some explicit type checks into using helper functions.

For some arithmetic assembler methods, instead of checking
IceType_i8 || IceType_i1, only allow IceType_i8 and assert if
an i1 leaked to that stage (should have been vetted earlier
by the bitcode reader / ABI checks). Could have looked up the
type width and isIntegerArithmeticType, etc. in the property table,
but that seemed a bit heavy for just checking one type
(or one of two types).

Also changed some f32 || f64 checks into just using
isScalarFloatingType() which looks things up in a property table.
Could alternatively just keep it as an simple f32 || f64 check,
and I could change isScalarFloatingType()'s implementation.

In some places where we assume something is either i32 or i64
and do a select, change that into using a helper function
so that we can do one compare, and then assert. Some of the
asserts are really redundant (already within a branch which
already checked that), but hopefully that disappears if
we compile in release mode.
Similar for f32 or f64 (which happened a lot in the assembler).

BUG=none
R=kschimpf@google.com, stichnot@chromium.org

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

[jvoung (off chromium), 2014-09-27 00:22:39]

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

[jvoung (off chromium), 2014-09-27 00:23:39]

https://codereview.chromium.org/613483002/diff/20001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/613483002/diff/20001/src/IceTypes.h#newcode87
src/IceTypes.h:87: inline bool isInt32Assuming32Or64(Type Ty) {
Hmm different name suggestions welcome

[Jim Stichnoth, 2014-09-27 00:38:22]

LGTM.  Your call on the naming choice...

https://codereview.chromium.org/613483002/diff/20001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/613483002/diff/20001/src/IceTypes.h#newcode87
src/IceTypes.h:87: inline bool isInt32Assuming32Or64(Type Ty) {
On 2014/09/27 00:23:39, jvoung wrote:
> Hmm different name suggestions welcome

I guess my suggestion would be
  isI32AssumingI32OrI64()
  isF32AssumingF32OrF64()

Another thought is to use Asserting instead of Assuming?  That might help convey
the possibly unexpected side effect of the isFoo() function, i.e. possibly
throwing an assertion failure.

[jvoung (off chromium), 2014-09-29 16:10:52]

https://codereview.chromium.org/613483002/diff/20001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/613483002/diff/20001/src/IceTypes.h#newcode87
src/IceTypes.h:87: inline bool isInt32Assuming32Or64(Type Ty) {
On 2014/09/27 00:38:22, stichnot wrote:
> On 2014/09/27 00:23:39, jvoung wrote:
> > Hmm different name suggestions welcome
> 
> I guess my suggestion would be
>   isI32AssumingI32OrI64()
>   isF32AssumingF32OrF64()
> 
> Another thought is to use Asserting instead of Assuming?  That might help
convey
> the possibly unexpected side effect of the isFoo() function, i.e. possibly
> throwing an assertion failure.

Going with "Asserting" instead of "Assuming"

[Karl, 2014-09-29 16:20:51]

kschimpf@google.com changed reviewers:
+ kschimpf@google.com

[Karl, 2014-09-29 16:20:51]

lgtm

https://codereview.chromium.org/613483002/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/613483002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2188: isFloat32Asserting32Or64(DestType) ?
"cvtsi64tof" : "cvtsi64tod",
Mnior nit. This conditional and the one one for "f" or "d" appear twice. Should
it be factored into an inline function?

[jvoung (off chromium), 2014-09-29 16:57:50]

https://codereview.chromium.org/613483002/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/613483002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2188: isFloat32Asserting32Or64(DestType) ?
"cvtsi64tof" : "cvtsi64tod",
On 2014/09/29 16:20:51, Karl wrote:
> Mnior nit. This conditional and the one one for "f" or "d" appear twice.
Should
> it be factored into an inline function?

I'm inclined to leave this alone for now if that's okay. It could be good to do
that with "Call the correct compiler-rt helper function" (the TODO below).

A quick scan shows that some of the functions will have different conventions:

float __floatsisf (int i) vs double __floatsidf (int i)

sf vs df, instead of f vs d. Hopefully that is consistent for the rest of the
compiler-rt functions.

[Karl, 2014-09-29 17:08:01]

lgtm

[jvoung (off chromium), 2014-09-29 17:16:06]

Committed patchset #3 (id:40001) manually as
3a56918360284f1329235055d59ff156f576de42 (presubmit successful).