Simplify NaN check in BaselineIC-x86-shared.cpp

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

Trunk
mozilla23
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
The code in ICCompare_Double::Compiler::generateStubCode makes an unconditional NaN check, which could be avoided after the fix for bug 869507.
(Assignee)

Updated

4 years ago
Depends on: 869507, 869525
(Assignee)

Comment 1

4 years ago
Created attachment 746493 [details] [diff] [review]
a patch containing a proposed fix
(Assignee)

Updated

4 years ago
No longer depends on: 869525
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 5

4 years ago
Created attachment 747177 [details] [diff] [review]
revised patch

Updated per review feedback.
Attachment #746493 - Attachment is obsolete: true
Attachment #747177 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

4 years ago
No longer depends on: 869507
(Assignee)

Updated

4 years ago
Blocks: 869507
(Assignee)

Comment 6

4 years ago
Created attachment 747185 [details] [diff] [review]
revised patch, this time with patch comments

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/integration/mozilla-inbound/rev/f6b920f5e96c
https://hg.mozilla.org/mozilla-central/rev/f6b920f5e96c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.