If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

IonMonkey: MIPS: Use conditional move in minMaxDouble/Float32

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: hev, Assigned: hev)

Tracking

unspecified
mozilla50
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8774610 [details] [diff] [review]
0001-Bug-1289261-IonMonkey-MIPS-Use-conditional-move-in-m.patch
Attachment #8774610 - Flags: review?(arai.unmht)
(Assignee)

Comment 2

a year ago
This is a small optimization, use mips float-point conditional move to reduce the branch instructions.
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.
Attachment #8774610 - Flags: review?(arai.unmht) → feedback+
(Assignee)

Comment 4

a year ago
Created attachment 8775003 [details] [diff] [review]
0001-Bug-1289261-IonMonkey-MIPS-Use-conditional-move-in-m.patch

Thank you!
Attachment #8774610 - Attachment is obsolete: true
Attachment #8775003 - Flags: review?(arai.unmht)
Attachment #8775003 - Flags: review?(arai.unmht) → review+

Comment 5

a year ago
Pushed by r@hev.cc:
https://hg.mozilla.org/integration/mozilla-inbound/rev/289855f3cc47
IonMonkey: MIPS: Use conditional move in minMaxDouble/Float32. r=arai

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/289855f3cc47
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.