Closed Bug 869532 Opened 8 years ago Closed 8 years ago

Simplify NaN check in BaselineIC-x86-shared.cpp

Categories

(Core :: JavaScript Engine, enhancement)

x86_64
All
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(1 file, 2 obsolete files)

The code in ICCompare_Double::Compiler::generateStubCode makes an unconditional NaN check, which could be avoided after the fix for bug 869507.
Depends on: 869507, 869525
No longer depends on: 869525
Blocks: 869525
Comment on attachment 746493 [details] [diff] [review]
a patch containing a proposed fix

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

This sounds good to me.

Just need to update the NaN_Unexpected by NaN_HandledByCond.
I will do this minor modification and push it for you.
Attachment #746493 - Flags: review+
Comment on attachment 746493 [details] [diff] [review]
a patch containing a proposed fix

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

::: js/src/ion/shared/BaselineIC-x86-shared.cpp
@@ +30,5 @@
> +    // Check for NaN, if needed.
> +    Assembler::NaNCond nanCond = Assembler::NaNCondFromDoubleCondition(cond);
> +    if (nanCond != Assembler::NaN_Unexpected) {
> +      masm.j(Assembler::NoParity, &notNaN);
> +      masm.moveValue(BooleanValue(nanCond == Assembler::NaN_IsTrue), dest);

nit: can you use masm.move(Int32(nanCond == Assembler::NaN_IsTrue), dest);

as we are packing the value after.
Attachment #746493 - Flags: review+ → feedback+
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Review of attachment 746493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > +      masm.moveValue(BooleanValue(nanCond == Assembler::NaN_IsTrue), dest);
> 
> nit: can you use masm.move(Int32(nanCond == Assembler::NaN_IsTrue), dest);
> 
> as we are packing the value after.

Thanks to jandem for catching this one.
Assignee: general → sunfish
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch revised patch (obsolete) — Splinter Review
Updated per review feedback.
Attachment #746493 - Attachment is obsolete: true
Attachment #747177 - Flags: review?(nicolas.b.pierron)
No longer depends on: 869507
Blocks: 869507
Sorry for all the trouble here. This patch now builds and passes tests on its own.
Attachment #747177 - Attachment is obsolete: true
Attachment #747177 - Flags: review?(nicolas.b.pierron)
Attachment #747185 - Flags: review?(nicolas.b.pierron)
Attachment #747185 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/f6b920f5e96c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.