Closed Bug 1040196 Opened 6 years ago Closed 6 years ago

Implement ES6 ToLength

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: gupta.rajagopal, Assigned: gupta.rajagopal)

References

()

Details

(Keywords: feature)

Attachments

(1 file, 6 obsolete files)

No description provided.
No longer blocks: 1039774
Blocks: 1039774
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
Attached patch Patch to implement ToLength v0 (obsolete) — Splinter Review
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!
Attachment #8459947 - Flags: feedback?(till)
Looks like Till is not available...

Jason, will you be able to help out, or is this something I should ask someone else?
Flags: needinfo?(jorendorff)
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.
Attachment #8459947 - Flags: feedback?(till) → feedback?(hv1989)
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
Attachment #8459947 - Flags: feedback?(hv1989)
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.)
Attached patch Patch to implement ToLength v1 (obsolete) — Splinter Review
This patch strips out the jit code - ToLength is self-hosted.
Attachment #8459947 - Attachment is obsolete: true
Attachment #8460522 - Flags: review?(jorendorff)
Blocks: 924058
Attached patch Patch to implement ToLength v2 (obsolete) — Splinter Review
Attachment #8460522 - Attachment is obsolete: true
Attachment #8460522 - Flags: review?(jorendorff)
Attachment #8460576 - Flags: review?(jorendorff)
Attached patch Patch to implement ToLength v3 (obsolete) — Splinter Review
Minor change: Added comment about the draft version.
Attachment #8460576 - Attachment is obsolete: true
Attachment #8460576 - Flags: review?(jorendorff)
Attachment #8460578 - Flags: review?(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.
Attachment #8460578 - Flags: review?(jorendorff) → review+
Attached patch Patch to implement ToLength v4 (obsolete) — Splinter Review
Attachment #8460578 - Attachment is obsolete: true
Attachment #8462669 - Flags: review+
Flags: needinfo?(jorendorff)
Attached patch Patch to implement ToLength v5 (obsolete) — Splinter Review
Try run fixes.

https://tbpl.mozilla.org/?tree=Try&rev=e853ffe398d4
Attachment #8462669 - Attachment is obsolete: true
Attachment #8464828 - Flags: review+
Keywords: checkin-needed
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.
Removed unused ToInteger variable in test.
Attachment #8464828 - Attachment is obsolete: true
Attachment #8464842 - Flags: review+
That try run has a single canceled build. Clearing checkin-needed until we a finished try run is posted.
Flags: needinfo?(gupta.rajagopal)
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=ec5534acd24a
Flags: needinfo?(gupta.rajagopal)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8a6391cb5180
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Is this fully covered by automatic tests?
Flags: in-testsuite?
Yes, it is.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.