Closed Bug 869507 Opened 7 years ago Closed 7 years ago
N checks in common floating-point comparisons on x86
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 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
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 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+
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
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 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+
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.