869507 - Redundant NaN checks in common floating-point comparisons on x86

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

x86's ucomisd can perform comparisons like DoubleLessThan with NaN properly, so there's no need for a separate NaN check for Javascript code like this (warning, contrived testcase):

var array = [ 0.1, undefined ];

for (var i = 0; i != array.length; ++i)
  if (array[i] < 0)
    print('hi');

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a patch containing a proposed fix

This patch eliminates NaN checks by changing NaNCondFromDoubleCondition to return NaN_Unexpected for conditions where the condition returned from ConditionFromDoubleCondition is sufficient. This is consistent with how NaNCondFromDoubleCondition is used, though admittedly it makes the name NaN_Unexpected a little misleading. Perhaps it should be renamed to NaN_NoCheckNeeded?

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 746464 [details] [diff] [review]
a patch containing a proposed fix

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

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +120,1 @@
>              return NaN_IsFalse;

It sounds like the meaning of it is more like:
  NaN_ImplicitFalse

because we still want to express the fact that NaN might be expected, and if we see it, then it alias the False case on the test condition in emitSet & emitBranch.

From intel documentation:
  unordered:     zf,pf,cf = 1,1,1
  greather_than: zf,pf,cf = 0,0,0
  less_than:     zf,pf,cf = 0,0,1
  equal:         zf,pf,cf = 1,0,0

DoubleOrdered => pf == 0 => !(pf == 1)
DoubleNotEqual => (zf == 0) => !(pf == 1)
DoubleGreaterThan => (zf == 0 && cf == 0) => !(pf == 1)
DoubleGreaterThanOrEqual => (cf == 0) => !(pf == 1)
DoubleLessThan === inverted DoubleGreaterThan
DoubleLessThanOrEqual === inverted DoubleGreaterThanOrEqual

DoubleOrdered => pf == 1 *does not implies* !(pf == 1)

Why did you added DoubleUnordered here? DoubleUnordered should remain as NaN_IsTrue.

Comment 3

[Dan Gohman [:sunfish]]

My observation was that callers of NaNCondFromDoubleCondition always insert a check based on ConditionFromDoubleCondition first. ConditionFromDoubleCondition for DoubleOrdered and DoubleUnordered returns NoParity and Parity, respectively, which are sufficient for those conditions. They don't need a separate NaN check.

I'll grant that the names are a little confusing this way though :-).

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman from comment #3)
> My observation was that callers of NaNCondFromDoubleCondition always insert
> a check based on ConditionFromDoubleCondition first.
> ConditionFromDoubleCondition for DoubleOrdered and DoubleUnordered returns
> NoParity and Parity, respectively, which are sufficient for those
> conditions. They don't need a separate NaN check.
> 
> I'll grant that the names are a little confusing this way though :-).

Indeed, ImplicitFalse let me thought that this case was wrong, but you are right and what you meant is that the default condition flags also account correctly for the NaN check, maybe something like:

  NaN_HandledByCond

would be easier to understand, as this patch remove the NaN checks when the outcome of the NaN checks are identical to the outcome of the condition flag.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: a revised patch, per feedback

Additional changes: NaN_Unexpected renamed to NaN_HandledByCond, comments added to NaNCondFromDoubleCondition and ConditionFromDoubleCondition, and more conditions added to the list that can omit the NaN check.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 746785 [details] [diff] [review]
a revised patch, per feedback

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

I checked each condition against the unordered case, and I confirm that the current modification is correct.
This is impressive that we can get rid of such number of NaN checks.

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +150,5 @@
>      static Condition InvertCondition(Condition cond);
>  
> +    // Return the primary condition to test. Some primary conditions may not
> +    // handle NaNs properly and may therefore require a secondary condition.
> +    // Use NaNCondFromDoubleCondition to determine what else is needed.

Thanks for the comments!

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97163e4941f2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for asserting and crashing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/948efc855b5b

https://tbpl.mozilla.org/php/getParsedLog.php?id=22739865&tree=Mozilla-Inbound

Comment 9

[Nicolas B. Pierron [:nbp]]

As philor noted in #developers, the following patch assert at the location which is modified by Bug 869532.

Assertion failure: nanCond == Assembler::NaN_IsTrue || nanCond == Assembler::NaN_IsFalse, at ../../../js/src/ion/shared/BaselineIC-x86-shared.cpp:37
TEST-UNEXPECTED-FAIL | automation.py | Exited with code 1 during test run

I suggest landing them both at the same time next time.

Comment 10

[Dan Gohman [:sunfish]]

Attachment: patch updated to apply after the fix for 869532

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 747186 [details] [diff] [review]
patch updated to apply after the fix for 869532

This patch is now meant to apply after the patch for 869532. I apologize for the mixup.

Comment 12

[Nicolas B. Pierron [:nbp]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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