Closed
Bug 995934
Opened 10 years ago
Closed 10 years ago
IonMonkey: math.round has extra code
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(2 files)
16.27 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
1.53 KB,
patch
|
Details | Diff | Splinter Review |
The first part of codegenerator of MRound looks like (on x86 only): > masm.loadConstantDouble(0.5, temp); > masm.xorpd(scratch, scratch); > masm.branchDouble(Assembler::DoubleLessThan, input, scratch, &negative); > Assembler::Condition bailCond = masm.testNegativeZero(input, output); > bailoutIf(bailCond, lir->snapshot()); Which translates in: > 0xb720ef41: movsd 0xb720f0c0,%xmm1 > 0xb720ef49: xorpd %xmm7,%xmm7 > 0xb720ef4d: ucomisd %xmm0,%xmm7 > 0xb720ef51: ja 0xb720ef8b > 0xb720ef57: xorpd %xmm7,%xmm7 > 0xb720ef5b: ucomisd %xmm7,%xmm0 > 0xb720ef5f: jne 0xb720ef6c So we do the "same" comparison twice and could have written: > 0xb720ef41: movsd 0xb720f0c0,%xmm1 > 0xb720ef49: xorpd %xmm7,%xmm7 > 0xb720ef4d: ucomisd %xmm0,%xmm7 > 0xb720ef51: ja 0xb720ef8b > 0xb720ef5f: jne 0xb720ef6c
Assignee | ||
Comment 1•10 years ago
|
||
This fixes the issue of the two compares in testNegativeZero and simplifies logic in there. 1) Rename testNegativeZeroXXX to branchNegativeZeroXXX. Doesn't do the stupid part of: > branch if not 0 to nonzero > ... > andl something to set flags > nonzero: > branch to bailout path if some flags are set but > branch if not 0 to nonzero > ... > branch if zero to failed label > nonzero: So we moved the second branch out of the hot path. 2) Move branchNegativeZeroXXX to shared x86 assembler. - branchNegativeZeroFloat32 has same code for x64 and x86. So no issue. - We want to use this function for convertDoubleToInt32 which is in shared x86 assembler. => Note: I could also duplicate convertDoubleToInt32 in both architectures. Arguable, but I think it doesn't matter at all. - Fix bug 770630 by doing this On small testcase: function test() { var a = 0 for (var i=0; i<100000000; i++) { a = Math.round(i+0.1); } return a } var start = new Date(); test() print(new Date() - start) I see time go from 220ms to 190ms.
Assignee: nobody → hv1989
Attachment #8410234 -
Flags: review?(benj)
Assignee | ||
Comment 2•10 years ago
|
||
This was part2. Where I would add branchZeroIsNegativeZero, which would only test if a 0 is a -0.0 or +0.0. Looking at performance we are not blocked by the computation but by the jump. So calculating it twice doesn't matter. Leaving it for now.
Comment 3•10 years ago
|
||
Comment on attachment 8410234 [details] [diff] [review] Part 1: Remove double compare Review of attachment 8410234 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing that! ::: js/src/jit/shared/MacroAssembler-x86-shared.cpp @@ +159,5 @@ > +MacroAssemblerX86Shared::branchNegativeZero(const FloatRegister ®, > + const Register &scratch, > + Label *label) > +{ > + // Determines whether the single double contained in the XMM register reg s/the single double/the low double @@ +160,5 @@ > + const Register &scratch, > + Label *label) > +{ > + // Determines whether the single double contained in the XMM register reg > + // is equal to double-precision -0. I'd remove "double-precision" here as -0 implicitly refers to a double, but up to you. @@ +176,5 @@ > + movmskpd(reg, scratch); > + > + // If reg is 1 or 3, input is negative zero. > + // If reg is 0 or 2, input is a normal zero. > + branchTest32(NonZero, scratch, Imm32(1), label); Nice! It makes much more sense. @@ +179,5 @@ > + // If reg is 0 or 2, input is a normal zero. > + branchTest32(NonZero, scratch, Imm32(1), label); > + > + bind(&nonZero); > +#elif defined(JS_CODEGEN_X64) It sort of doesn't make sense to have these #ifdef here, unless you have in mind the future work consisting in merging MacroAssemblers into a single one. I'd keep these in the arch specific MacroAssemblers in the meanwhile.
Attachment #8410234 -
Flags: review?(benj) → review+
Assignee | ||
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b188b044cc
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f4b188b044cc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•