1040196 - Implement ES6 ToLength

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[guptha]

Attachment: Patch to implement ToLength v0

The self-hosted part of this patch works fine.

I have had difficulties inlining this though (I've not implemented much inside the jits before - so, most of the patch was from aping existing functions, and not with full understanding of everything)

1. I am not sure how to copy from one FloatRegister to another in CodeGenerator.cpp
2. I get an assertion failure when I do "var ToLength = getSelfHostedValue('ToLength'); var a;for (var i = 0; i < 10000; i++) { a = ToLength(100);}"

The back trace for this is:
(lldb) bt
* thread #2: tid = 0x8a31e2, 0x00000001004e7827 js`js::jit::VirtualRegister::init(this=0x0000000103043500, alloc=0x000000010302d020, block=0x000000010303f320, ins=0x0000000103040b50, def=0x0000000103040bb0, isTemp=true) + 151 at LiveRangeAllocator.h:451, name = 'Analysis Helper', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001004e7827 js`js::jit::VirtualRegister::init(this=0x0000000103043500, alloc=0x000000010302d020, block=0x000000010303f320, ins=0x0000000103040b50, def=0x0000000103040bb0, isTemp=true) + 151 at LiveRangeAllocator.h:451
    frame #1: 0x00000001004dc43b js`js::jit::LiveRangeAllocator<js::jit::LinearScanVirtualRegister, true>::init(this=0x0000000101e806d0) + 1147 at LiveRangeAllocator.cpp:538
    frame #2: 0x00000001004bc47b js`js::jit::LiveRangeAllocator<js::jit::LinearScanVirtualRegister, true>::buildLivenessInfo(this=0x0000000101e806d0) + 59 at LiveRangeAllocator.cpp:594
    frame #3: 0x0000000100461965 js`js::jit::LinearScanAllocator::go(this=0x0000000101e806d0) + 53 at LinearScan.cpp:1291
    frame #4: 0x000000010033dec6 js`js::jit::GenerateLIR(mir=0x000000010302d260) + 710 at Ion.cpp:1606
    frame #5: 0x000000010033e42c js`js::jit::CompileBackEnd(mir=0x000000010302d260) + 92 at Ion.cpp:1694
    frame #6: 0x000000010084c631 js`js::HelperThread::handleIonWorkload(this=0x0000000103018e00) + 1041 at HelperThreads.cpp:967
    frame #7: 0x000000010084bda4 js`js::HelperThread::threadLoop(this=0x0000000103018e00) + 596 at HelperThreads.cpp:1266
    frame #8: 0x000000010084a947 js`js::HelperThread::ThreadMain(arg=0x0000000103018e00) + 39 at HelperThreads.cpp:872
    frame #9: 0x0000000101a84a07 libnspr4.dylib`___lldb_unnamed_function141$$libnspr4.dylib + 211
    frame #10: 0x00007fff847b3899 libsystem_pthread.dylib`_pthread_body + 138
    frame #11: 0x00007fff847b372a libsystem_pthread.dylib`_pthread_start + 137

Not entirely sure what I'm doing wrong. Any help will be much appreciated!

Comment 2

[guptha]

Looks like Till is not available...

Jason, will you be able to help out, or is this something I should ask someone else?

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8459947 [details] [diff] [review]
Patch to implement ToLength v0

I'm on pto, and i don't know too much about the jits either, so forwarding the request to hannes.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8459947 [details] [diff] [review]
Patch to implement ToLength v0

Review of attachment 8459947 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. The lowering/codegeneration are the hardest part. So that is normal to make mistakes here. I've added comments there. Feel free to ask more questions if I didn't explain it properly enough.

::: js/src/jit/CodeGenerator.cpp
@@ +175,5 @@
> +    FloatRegister output = ToFloatRegister(lir->output());
> +    FloatRegister input = ToFloatRegister(lir->number());
> +    Label positive, lessThanMax;
> +
> +    FloatRegister zero;

This is a wrong way to get a (temp) register. 
You need to ask the "Register Allocator" for a register to use.
(see my comment next to LToLength)

@@ +179,5 @@
> +    FloatRegister zero;
> +    masm.loadConstantDouble(0.0, zero);
> +    masm.branchDouble(Assembler::DoubleGreaterThan, input, zero, &positive);
> +    masm.loadConstantDouble(0.0, output);
> +    return true;

This return looks wrong.
We are generating code that the computer eventually execute. All code it has to execute has "masm.xxx". The "return true". Will just stop this function generating the code. So the code after this "return" isn't run and the intended code that should get generated is not generated. 
In order to let the code know everything is done, you need to jump to the end, instead:

Label positive, done;

masm.branchDouble(...., &positive);
masm.loadConstantDouble(0.0, output);
masm.jump(&done);

masm.bind(&positive);
masm.loadConstantDouble(1.0, output);

masm.bind(&done);

::: js/src/jit/LIR-Common.h
@@ +3006,5 @@
>          return mir_->toToFloat32();
>      }
>  };
>  
> +class LToLength: public LInstructionHelper<1, 1, 2>

You request 2 temp registers, but you don't initialize them.
e.g. look at LRound, requesting 1 temp register:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/LIR-Common.h#4883
and initializing it:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Lowering.cpp#1241

Comment 5

[Hannes Verschore [:h4writer]]

Note: I don't know how ToLength works at all. So there might be spec errors because of it or suboptimal code. (i.e. where will ToLength mostly get used? Will we mostly use it with Int32 results or mostly with Doubles? Currently it is optimized for the later.)

Comment 6

[guptha]

Attachment: Patch to implement ToLength v1

This patch strips out the jit code - ToLength is self-hosted.

Comment 7

[guptha]

Attachment: Patch to implement ToLength v2

Comment 8

[guptha]

Attachment: Patch to implement ToLength v3

Minor change: Added comment about the draft version.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8460578 [details] [diff] [review]
Patch to implement ToLength v3

Review of attachment 8460578 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks. r=me with test tweaks fixed.

::: js/src/builtin/Utilities.js
@@ +150,5 @@
> +    if (v <= 0)
> +        return 0;
> +
> +    // Math.pow(2, 53) - 1 = 9007199254740991
> +    return v < 9007199254740991 ? v : 9007199254740991;

I think it'd be more obvious if you wrote 0x1fffffffffffff, but your call.

::: js/src/tests/ecma_6/toLength.js
@@ +19,5 @@
> +assertEq(ToLength(0), ToInteger(0));
> +assertEq(ToLength(1), ToInteger(1));
> +assertEq(ToLength(2), ToInteger(2));
> +assertEq(ToLength(3.3), ToInteger(3.3));
> +assertEq(ToLength(10/3), ToInteger(10/3));

Please ditch ToInteger on the right-hand side here and just specify the exact expected value (0, 1, 2, 3, and 3).

@@ +30,5 @@
> +assertEq(ToLength(Math.pow(2,53)), ToInteger(Math.pow(2,53) - 1));
> +assertEq(ToLength(Math.pow(2,53) + 1), ToInteger(Math.pow(2,53) - 1));
> +assertEq(ToLength(Math.pow(2,54)), ToInteger(Math.pow(2,53) - 1));
> +assertEq(ToLength(Math.pow(2,64)), ToInteger(Math.pow(2,53) - 1));
> +assertEq(ToLength(Infinity), ToInteger(Math.pow(2,53) - 1));

Same down here, and if you could do this beforehand

var maxLength = Math.pow(2, 53) - 1;

then it would make the assertions easier to read.

Comment 10

[guptha]

Attachment: Patch to implement ToLength v4

Comment 11

[guptha]

Attachment: Patch to implement ToLength v5

Try run fixes.

https://tbpl.mozilla.org/?tree=Try&rev=e853ffe398d4

Comment 12

[Tom S [:evilpie]]

Comment on attachment 8464828 [details] [diff] [review]
Patch to implement ToLength v5

Review of attachment 8464828 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/toLength.js
@@ +4,5 @@
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +var ToLength = getSelfHostedValue('ToLength');
> +var ToInteger = getSelfHostedValue('ToInteger');

This seems unused.

Comment 13

[guptha]

Attachment: Patch to implement ToLength v6

Removed unused ToInteger variable in test.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

That try run has a single canceled build. Clearing checkin-needed until we a finished try run is posted.

Comment 15

[guptha]

https://tbpl.mozilla.org/?tree=Try&rev=ec5534acd24a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a6391cb5180

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a6391cb5180

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is this fully covered by automatic tests?

Comment 19

[guptha]

Yes, it is.