Closed Bug 848510 Opened 7 years ago Closed 7 years ago

BaselineCompiler: Add BITNOT stub for doubles

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jandem, Assigned: isk)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 3 obsolete files)

Some Emscripten tests use ~~x to ensure x is int32. We should add a stub to handle BITNOT on doubles to avoid a slow VM call.
Attached patch bug848510.patch (obsolete) — Splinter Review
I add branch in visitBitNot.
BitNot of VMFunction (Interpreter-inl.h) convert values into int32.
So I think this patch is safe.
Assignee: general → iseki.m.aa
Status: NEW → ASSIGNED
Attachment #8341585 - Flags: review?(jdemooij)
Comment on attachment 8341585 [details] [diff] [review]
bug848510.patch

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

In LIRGenerator::visitBitNot, we should have converted any double inputs to int32 already so this doesn't change anything.

This bug is about fixing the Baseline JIT, in BaselineIC.cpp. We should add a new stub (ICUnaryArith_Double), like the existing ICUnaryArith_Int32 one that converts the input from double to int32 and then does a bitwise-not.
Attachment #8341585 - Flags: review?(jdemooij)
Attached patch bug848510.patch (obsolete) — Splinter Review
Thanks for your reviewing.

I had not understand the mean of stub.
I add stub for double.
Attachment #8341585 - Attachment is obsolete: true
Attachment #8342136 - Flags: review?(jdemooij)
Attached patch bug848510.patch (obsolete) — Splinter Review
I add stub in ICUnaryArith_Double::Compiler::generateStubCode and not operation.
Attachment #8342136 - Attachment is obsolete: true
Attachment #8342136 - Flags: review?(jdemooij)
Attachment #8342289 - Flags: review?(jdemooij)
Comment on attachment 8342289 [details] [diff] [review]
bug848510.patch

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

Looks good!

Just for fun, can you try an opt build and see how much the following script gets faster with your patch? :)

function f(x) {
    try {} finally {}; // Don't Ion-compile.
    for (var i=0; i<100000000; i++)
        var res = ~x;
    return res;
}
print(f(10.5));

::: js/src/jit/BaselineIC.cpp
@@ +3111,5 @@
> +        masm.negateDouble(FloatReg0);
> +        masm.boxDouble(FloatReg0, R0);
> +    } else {
> +        // Truncate the double to an int32.
> +        Register intReg;

Remove intReg and the push(intReg) / pop(intReg) below.

@@ +3112,5 @@
> +        masm.boxDouble(FloatReg0, R0);
> +    } else {
> +        // Truncate the double to an int32.
> +        Register intReg;
> +        Register scratchReg;

Register scratchReg = R1.scratchReg();

@@ +3128,5 @@
> +        masm.storeCallResult(scratchReg);
> +        masm.pop(intReg);
> +
> +        masm.bind(&doneTruncate);
> +        masm.not32(intReg, scratchReg);

masm.not32(scratchReg)

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +153,5 @@
>      void xor32(Imm32 imm, Register dest) {
>          xorl(imm, dest);
>      }
> +    void not32(Register src, Register dest) {
> +        notl(src);

We're not using the dest register, so we should use one argument, |Register reg|, to avoid confusion.

For ARM, you can just do ma_mvn(reg, reg)
Attachment #8342289 - Flags: review?(jdemooij)
Attached patch bug848510.patchSplinter Review
I fixed the code which pointed out.

I run the following script with the patch and without the patch.

function f(x) {
    try {} finally {}; // Don't Ion-compile.
    var t = new Date;
    for (var i=0; i<10000000; i++)
        var res = ~x;
    print(new Date - t);
    return res;
}
print(f(10.5));

before: 885
after: 630

slightly fast.
Attachment #8342289 - Attachment is obsolete: true
Attachment #8342846 - Flags: review?(jdemooij)
Comment on attachment 8342846 [details] [diff] [review]
bug848510.patch

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

Looks good, but you forgot to remove the "op == JSOP_NEG" in DoUnaryArithFallback, we need that to use the new code you added. Can you see how much that helps and if it passes jit_test.py --ion with that change? I will r+ and land it then :)
Attachment #8342846 - Flags: review?(jdemooij)
Attached patch bug848510.patchSplinter Review
Thank you for your reviewing.

I remove "op == JSOP_NEG".
The result is following.

before: 885
after: 129

>Can you see how much that helps and if it passes jit_test.py --ion with that change?
jit_test.py is js/src/tests/jstests.py?
If so, this patch pass the tests in without --ion.
I don't know how to test in ion.
Would you tell me how to test in ion?
Attachment #8344458 - Flags: review?(jdemooij)
Comment on attachment 8344458 [details] [diff] [review]
bug848510.patch

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

Looks great, thanks! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c17264cd2d1

(In reply to iseki.m.aa from comment #8)
> jit_test.py is js/src/tests/jstests.py?
> If so, this patch pass the tests in without --ion.
> I don't know how to test in ion.
> Would you tell me how to test in ion?

You can run: jit-test/jit_test.py --ion js
Attachment #8344458 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/5c17264cd2d1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.