Closed Bug 869507 Opened 7 years ago Closed 7 years ago

Redundant NaN checks in common floating-point comparisons on x86

Categories

(Core :: JavaScript Engine, enhancement)

x86_64
All
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(1 file, 2 obsolete files)

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');
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?
Blocks: 869532
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.
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 :-).
(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.
Assignee: general → sunfish
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch a revised patch, per feedback (obsolete) — Splinter Review
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.
Attachment #746464 - Attachment is obsolete: true
Attachment #746785 - Flags: review?(nicolas.b.pierron)
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!
Attachment #746785 - Flags: review?(nicolas.b.pierron) → review+
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.
No longer blocks: 869532
Depends on: 869532
Attachment #746785 - Attachment is obsolete: true
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.
Attachment #747186 - Flags: review?(nicolas.b.pierron)
Attachment #747186 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/c1ee14175d13
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.