1289261 - IonMonkey: MIPS: Use conditional move in minMaxDouble/Float32

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[hev [:hev]]

Attachment: 0001-Bug-1289261-IonMonkey-MIPS-Use-conditional-move-in-m.patch

Comment 2

[hev [:hev]]

This is a small optimization, use mips float-point conditional move to reduce the branch instructions.

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 8774610 [details] [diff] [review]
0001-Bug-1289261-IonMonkey-MIPS-Use-conditional-move-in-m.patch

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

Thank you for the explanation :)

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +972,5 @@
>      // First or second is NaN, result is NaN.
>      ma_bc1d(first, second, &nan, Assembler::DoubleUnordered, ShortJump);
>      // Make sure we handle -0 and 0 right.
>      ma_bc1d(first, second, &equal, Assembler::DoubleEqual, ShortJump);
> +    compareFloatingPoint(DoubleFloat, first, second, cond, &moveCondition);

it would be better asserting |moveCondition == TestForTrue| here.
same for Float32.

@@ +979,5 @@
>  
>      // Check for zero.
>      bind(&equal);
>      asMasm().loadConstantDouble(0.0, ScratchDoubleReg);
>      // First wasn't 0 or -0, so just return it.

This comment doesn't make sense here.
Can you rephrase or move it to match to what the code does?
Same for Float32

@@ +981,5 @@
>      bind(&equal);
>      asMasm().loadConstantDouble(0.0, ScratchDoubleReg);
>      // First wasn't 0 or -0, so just return it.
> +    compareFloatingPoint(DoubleFloat, first, ScratchDoubleReg,
> +                         Assembler::DoubleNotEqualOrUnordered, &moveCondition);

Now we perform "move" when first is 0 or -0,
so it would be simpler to use Assembler::DoubleEqual,
and assert |moveCondition == TestForTrue|  before |as_movt|.
same for Float32.

Comment 4

[hev [:hev]]

Attachment: 0001-Bug-1289261-IonMonkey-MIPS-Use-conditional-move-in-m.patch

Thank you!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/289855f3cc47
IonMonkey: MIPS: Use conditional move in minMaxDouble/Float32. r=arai

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/289855f3cc47