Remove unnecessary checks and runtime calls from ToLength

Remove unnecessary checks and runtime calls from ToLength

This should make it easier to move to using ToLength in more places to
meet ES2015 spec requirements without regressing performance. We might
also want to mark it as inlineable, but I've left that for another
patch.

Also pull 2^53 - 1 out into a macro constant instead of requiring a load
from Number.MAX_SAFE_INTEGER.

[adamk, 2015-08-24 22:42:17]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org

[adamk, 2015-08-24 22:42:17]

bmeurer, this might be up your alley. CC littledan as he ran into perf issues
with ToLength previously.

https://codereview.chromium.org/1314493005/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1314493005/diff/1/src/runtime.js#newcode737
src/runtime.js:737: arg = TO_NUMBER_INLINE(arg);
Not sure what the state of the art is...should this be "arg = +arg"? (or should
we add a TO_NUMBER macro that does +(arg)?)

[Benedikt Meurer, 2015-08-25 04:53:48]

Hey Adam,

I'm currently working on implementing ToPrimitive for ES6, in a way that we can
have %_ToPrimitive, %_ToNumber and %_ToString (with proper code stubs and also
proper support for those within the runtime without resorting to
Execution::ToNumber and friends). Once this is done, it should be easy to add
%_ToInteger and %_ToLength on top and then we can remove the helpers from
runtime.js completely.
But that'll take time, so landing your patch still makes sense to make progress
now. I'm not sure about the test failures, but I'd say the patch is LGTM.

[adamk, 2015-08-25 21:20:15]

On 2015/08/25 04:53:48, Benedikt Meurer wrote:
> Hey Adam,
> 
> I'm currently working on implementing ToPrimitive for ES6, in a way that we
can
> have %_ToPrimitive, %_ToNumber and %_ToString (with proper code stubs and also
> proper support for those within the runtime without resorting to
> Execution::ToNumber and friends). Once this is done, it should be easy to add
> %_ToInteger and %_ToLength on top and then we can remove the helpers from
> runtime.js completely.
> But that'll take time, so landing your patch still makes sense to make
progress
> now. I'm not sure about the test failures, but I'd say the patch is LGTM.

Thanks for the background, and I look forward to the future you describe.

In the meantime, most of the failures were due to a missing NaN check (which
I've added). But I'm getting an odd failure in the
Reflect.apply/Reflect.construct tests (looks like it might be an infinite
recursion); still investigating.

[adamk, 2015-08-25 21:55:42]

https://codereview.chromium.org/1314493005/diff/20001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1314493005/diff/20001/src/runtime.js#newcode739
src/runtime.js:739: arg = %_MathFloor(arg);
This line is the problem. I get a crash if |arg| is a Smi. Repro:

d8 --harmony-reflect --turbo --always-opt -e 'Reflect.apply(function(){}, {}, {
length: 1 })'

Even more bizarrely, adding a Smi check results in a crash under --stress-opt
--always-opt on a different case:

function f() { 'use strict'; }
Reflect.apply(f, undefined, {});
for (var i = 0; i < 256; ++i) {
  Reflect.apply(f, undefined, { length: i });
}

Seems like something is particularly screwy with either the Reflect stuff or
MathFloor (both of which have strange implementations).