869532 - Simplify NaN check in BaselineIC-x86-shared.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

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]]

Attachment: a patch containing a proposed fix

Comment 2

[Nicolas B. Pierron [:nbp]]

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]]

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]]

(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]]

Attachment: revised patch

Updated per review feedback.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: 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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b920f5e96c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6b920f5e96c