Closed
Bug 931489
Opened 9 years ago
Closed 9 years ago
use range analysis to eliminate NaN code
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: sunfish, Assigned: sunfish)
Details
(Whiteboard: [qa-])
Attachments
(3 files)
6.13 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
5.01 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
4.83 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
We can use range analysis to eliminate several NaN and negative-zero checks. The following are a series of patches to implement this for a few things. The main one that's interesting is the one for MCompare. The other two, for MNot and MPowHalf, are less interesting so feel free to decline them if you don't think they're worth the bother.
Attachment #822919 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #822922 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #822924 -
Flags: review?(nicolas.b.pierron)
Comment 3•9 years ago
|
||
Comment on attachment 822919 [details] [diff] [review] range-ordered-compare.patch Review of attachment 822919 [details] [diff] [review]: ----------------------------------------------------------------- Rename the flag to canBeNaN (don't forget to invert the meaning of it), and r=me. I wonder how we managed to keep this around for a while. ::: js/src/jit/MIR.h @@ +2140,5 @@ > private: > CompareType compareType_; > JSOp jsop_; > bool operandMightEmulateUndefined_; > + bool alwaysOrdered_; alwaysOrdered_ is not clear if you are thinking in terms of JavaScript, I think it makes more sense to rename it canBeNaN_.
Attachment #822919 -
Flags: review?(nicolas.b.pierron) → review+
Comment 4•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #3) > Comment on attachment 822919 [details] [diff] [review] > range-ordered-compare.patch > > Review of attachment 822919 [details] [diff] [review]: > ----------------------------------------------------------------- > alwaysOrdered_ is not clear if you are thinking in terms of JavaScript, I > think it makes more sense to rename it canBeNaN_. After looking at the second patch, I think it makes more sense to use the same naming as in range-pow-half (operandsAreNeverNaN_), as we are not dealing with the return type of the MCompare.
Comment 5•9 years ago
|
||
Comment on attachment 822922 [details] [diff] [review] range-pow-half.patch Review of attachment 822922 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp @@ +477,4 @@ > > + Assembler::DoubleCondition cond = Assembler::DoubleNotEqualOrUnordered; > + if (ins->mir()->operandIsNeverNaN()) > + cond = Assembler::DoubleNotEqual; Is that a performance issue? Can we just avoid adding this extra flag and always used the …OrUnordered condition?
Attachment #822922 -
Flags: review?(nicolas.b.pierron)
Updated•9 years ago
|
Attachment #822924 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #4) > After looking at the second patch, I think it makes more sense to use the > same naming as in range-pow-half (operandsAreNeverNaN_), as we are not > dealing with the return type of the MCompare. Done. https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ff16009983 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d175be0fcb0
Whiteboard: [leave open]
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #5) > Comment on attachment 822922 [details] [diff] [review] > range-pow-half.patch > > Review of attachment 822922 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp > @@ +477,4 @@ > > > > + Assembler::DoubleCondition cond = Assembler::DoubleNotEqualOrUnordered; > > + if (ins->mir()->operandIsNeverNaN()) > > + cond = Assembler::DoubleNotEqual; > > Is that a performance issue? > Can we just avoid adding this extra flag and always used the …OrUnordered > condition? Yes, it is a performance issue. DoubleNotEqual is more efficient than DoubleNotEqualOrUnordered on x86/x64 because it doesn't seed a separate NaN check.
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e9ff16009983 https://hg.mozilla.org/mozilla-central/rev/3d175be0fcb0
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 822922 [details] [diff] [review] range-pow-half.patch (In reply to Dan Gohman [:sunfish] from comment #7) > (In reply to Nicolas B. Pierron [:nbp] from comment #5) > > Comment on attachment 822922 [details] [diff] [review] > > range-pow-half.patch > > > > Review of attachment 822922 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp > > @@ +477,4 @@ > > > > > > + Assembler::DoubleCondition cond = Assembler::DoubleNotEqualOrUnordered; > > > + if (ins->mir()->operandIsNeverNaN()) > > > + cond = Assembler::DoubleNotEqual; > > > > Is that a performance issue? > > Can we just avoid adding this extra flag and always used the …OrUnordered > > condition? > > Yes, it is a performance issue. DoubleNotEqual is more efficient than > DoubleNotEqualOrUnordered on x86/x64 because it doesn't seed a separate NaN > check. The question answered, I'm re-requesting review, so I don't loose track of this. It's not urgent.
Attachment #822922 -
Flags: review?(nicolas.b.pierron)
Updated•9 years ago
|
Attachment #822922 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d286c5eecc
Whiteboard: [leave open]
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/91d286c5eecc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•9 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•