Last Comment Bug 869532 - Simplify NaN check in BaselineIC-x86-shared.cpp
: Simplify NaN check in BaselineIC-x86-shared.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 All
: -- enhancement (vote)
: mozilla23
Assigned To: Dan Gohman [:sunfish]
:
Mentors:
Depends on:
Blocks: 869507 869525
  Show dependency treegraph
 
Reported: 2013-05-07 10:03 PDT by Dan Gohman [:sunfish]
Modified: 2013-05-08 21:30 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a patch containing a proposed fix (1.82 KB, patch)
2013-05-07 10:05 PDT, Dan Gohman [:sunfish]
nicolas.b.pierron: feedback+
Details | Diff | Review
revised patch (1.62 KB, patch)
2013-05-08 16:01 PDT, Dan Gohman [:sunfish]
no flags Details | Diff | Review
revised patch, this time with patch comments (1.81 KB, patch)
2013-05-08 16:13 PDT, Dan Gohman [:sunfish]
nicolas.b.pierron: review+
Details | Diff | Review

Description Dan Gohman [:sunfish] 2013-05-07 10:03:11 PDT
The code in ICCompare_Double::Compiler::generateStubCode makes an unconditional NaN check, which could be avoided after the fix for bug 869507.
Comment 1 Dan Gohman [:sunfish] 2013-05-07 10:05:59 PDT
Created attachment 746493 [details] [diff] [review]
a patch containing a proposed fix
Comment 2 Nicolas B. Pierron [:nbp] 2013-05-08 11:36:28 PDT
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.
Comment 3 Nicolas B. Pierron [:nbp] 2013-05-08 11:44:18 PDT
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.
Comment 4 Nicolas B. Pierron [:nbp] 2013-05-08 11:45:31 PDT
(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.
Comment 5 Dan Gohman [:sunfish] 2013-05-08 16:01:52 PDT
Created attachment 747177 [details] [diff] [review]
revised patch

Updated per review feedback.
Comment 6 Dan Gohman [:sunfish] 2013-05-08 16:13:00 PDT
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.
Comment 7 Nicolas B. Pierron [:nbp] 2013-05-08 16:43:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b920f5e96c
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-05-08 21:30:49 PDT
https://hg.mozilla.org/mozilla-central/rev/f6b920f5e96c

Note You need to log in before you can comment on or make changes to this bug.