855514 - IonMonkey: add fastpath for JSOP_TOID of a double that fits in an int32

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

We hit this in jpeg2000 a lot. The input to JSOP_TOID is typed to be int/float. Therefore we don't do the fastpath of int, but the slow vmcall. But actually the type is a double that fits nicely in an int.

I'll add a LDoubleToIdV that tries to fit the double in an int and upon fail does the vmcall in an ool path.

This increases our score from 4.6s to 2.5s.
That makes us faster than JM+TI (3.5s) and V8 (2.8s).

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Add fastpath for double inputs to ToId

Adds a fastpath if input is double that fits into an int.

Comment 2

[Tom S. (please needinfo tschuster)]

Comment on attachment 733838 [details] [diff] [review]
Add fastpath for double inputs to ToId

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

::: js/src/ion/CodeGenerator.cpp
@@ +5245,5 @@
> +}
> +bool
> +CodeGenerator::visitToIdV(LToIdV *lir)
> +{
> +    Label done, vm;

Unused.

@@ +5251,5 @@
> +    FloatRegister temp = ToFloatRegister(lir->tempFloat());
> +    const ValueOperand out = ToOutValue(lir);
> +
> +    OutOfLineToIdV *ool = new OutOfLineToIdV(lir);
> +    if (!addOutOfLineCode(ool))

Could this use oolCallVM?

@@ +5255,5 @@
> +    if (!addOutOfLineCode(ool))
> +        return false;
> +
> +    ValueOperand index = ToValue(lir, LToIdV::Index);
> +    Register tag = masm.splitTagForTest(index);

You don't need to split if you only use the tag once.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Add fastpath for double inputs to ToId

Addressed the comments of evilpie, thanks :D. I had no clue oolCallVM existed. Decreases the code touched a lot :D

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 733844 [details] [diff] [review]
Add fastpath for double inputs to ToId

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

Looks good, but could use a jit-test. r=me with that and the comments below addressed.

::: js/src/ion/CodeGenerator.cpp
@@ +5213,5 @@
> +                                    ImmGCPtr(current->mir()->info().script()),
> +                                    ImmWord(lir->mir()->resumePoint()->pc()),
> +                                    ToValue(lir, LToIdV::Object),
> +                                    ToValue(lir, LToIdV::Index)),
> +                                   StoreValueTo(out));

Nit: indentation is off here (tabs?).

@@ +5215,5 @@
> +                                    ToValue(lir, LToIdV::Object),
> +                                    ToValue(lir, LToIdV::Index)),
> +                                   StoreValueTo(out));
> +
> +    masm.branchTestDouble(Assembler::NotEqual, index, ool->entry());

The int32 case is probably also common (if the index is either int32-or-double) and easy to support. Can you add something like this here:

Label notInt32;
masm.branchTestInt32(Assembler::NotEqual, index, &notInt32);
masm.moveValue(index, out);
masm.jump(ool->rejoin());
masm.bind(&notInt32);

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Add fastpath for double inputs to ToId

I'll have to ask another review round. I did the Int32 moveValue, but there is an issue. moveValue fails for 32bit when input is (a:b) and output (b:a).
(a:b) is notation for the (type:payload) register.

Therefore I used tagValue in that case. I also had to adjust tagValue, as it also didn't like this behaviour, but by rearranging how the mov's happen, fixed that.

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 734175 [details] [diff] [review]
Add fastpath for double inputs to ToId

Following a small conversation on IRC, we went for the previous version and changed useBoxAtStart to useBox. That solves the issues too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/11f52a74cac4

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11f52a74cac4