Last Comment Bug 869507 - Redundant NaN checks in common floating-point comparisons on x86
: Redundant NaN checks in common floating-point comparisons on x86
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: 869532
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-07 09:23 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.36 KB, patch)
2013-05-07 09:28 PDT, Dan Gohman [:sunfish]
no flags Details | Diff | Splinter Review
a revised patch, per feedback (5.59 KB, patch)
2013-05-07 23:21 PDT, Dan Gohman [:sunfish]
nicolas.b.pierron: review+
Details | Diff | Splinter Review
patch updated to apply after the fix for 869532 (6.50 KB, patch)
2013-05-08 16:15 PDT, Dan Gohman [:sunfish]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description Dan Gohman [:sunfish] 2013-05-07 09:23:45 PDT
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] 2013-05-07 09:28:42 PDT
Created attachment 746464 [details] [diff] [review]
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] 2013-05-07 11:55:31 PDT
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] 2013-05-07 15:35:26 PDT
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] 2013-05-07 16:20:21 PDT
(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] 2013-05-07 23:21:59 PDT
Created attachment 746785 [details] [diff] [review]
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] 2013-05-08 11:17:40 PDT
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] 2013-05-08 11:30:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/97163e4941f2
Comment 9 Nicolas B. Pierron [:nbp] 2013-05-08 12:01:58 PDT
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] 2013-05-08 16:15:30 PDT
Created attachment 747186 [details] [diff] [review]
patch updated to apply after the fix for 869532
Comment 11 Dan Gohman [:sunfish] 2013-05-08 16:19:38 PDT
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] 2013-05-08 16:43:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ee14175d13
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-05-08 21:30:42 PDT
https://hg.mozilla.org/mozilla-central/rev/c1ee14175d13

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