Closed
Bug 728852
Opened 12 years ago
Closed 12 years ago
IonMonkey: miscellaneous improvements to the Double->Int32 handling on ARM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file)
11.25 KB,
patch
|
jbramley
:
review+
|
Details | Diff | Splinter Review |
I found two things that were slowing us down on Sunspider: 1) the implementation of round called emitDoubleToInt32, rather than emitTruncateDouble 2) ValueToInt32 bails out if emitTruncateDouble fails The attached patch should address both of these issues (not in the most elegant manner, but...)
Attachment #598836 -
Flags: review?(Jacob.Bramley)
Comment 1•12 years ago
|
||
Comment on attachment 598836 [details] [diff] [review] /home/mrosenberg/patches/optimize_double_to_int-r1.patch Review of attachment 598836 [details] [diff] [review]: ----------------------------------------------------------------- There's one actual (but trivial) bug to be fixed, and a set of comments which are mostly tidying and other such things. ::: js/src/ion/arm/CodeGenerator-arm.cpp @@ +859,5 @@ > // +2 +1.5 +1 +0.5 +0 -0.5 -1 -1.5 -2 > // vcvt: }-------> }-------><-------{ <-------{ > // floor: }-------> }-------> }-------> }-------> > > + //masm.ma_vsub(ScratchFloatReg, ScratchFloatReg, ScratchFloatReg); This line can go. @@ +883,5 @@ > // In case of impossible convertion, INT_MIN is stored in output, which > // cause an overflow. > if (!bailoutIf(Assembler::Overflow, lir->snapshot())) > return false; > +#endif Delete the #if 0 block, or make it a run-time assertion (if we can do those). @@ +925,5 @@ > + masm.ma_b(fail, Assembler::Equal); > +} > + > +static uint32 > +js_DoubleToECMAInt32NonInline(jsdouble d) There's an optimized version of this in jsnum.h ("js_DoubleToECMAInt32"). The function is (C++) inline, but you could simply call that function from here to get a non-inline version of it. The implementation in this function is the same as the fall-back (slow) case from js_DoubleToECMAInt32. @@ +959,5 @@ > masm.ma_cmp(dest, Imm32(0x7fffffff)); > masm.ma_cmp(dest, Imm32(0x80000000), Assembler::NotEqual); > + Label join; > + masm.ma_b(&join, Assembler::NotEqual); > + // oh god, i'm a bad person, this is so the label is referenced :( Did you do it to avoid a warning? I don't understand why it's needed, since you reference the label at the end anyway. Is this development code? ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +967,5 @@ > > void > +MacroAssemblerARM::ma_vneg(FloatRegister src, FloatRegister dest) > +{ > + as_vmov(dest, src); s/as_vmov/as_vneg/ @@ +1019,5 @@ > void > MacroAssemblerARM::ma_vxfer(FloatRegister src, Register dest) > { > as_vxfer(dest, InvalidReg, VFPRegister(src).singleOverlay(), FloatToCore); > +}void You definitely need some white-space there! @@ +1585,5 @@ > } > > +// This functon almost certainly should not be called. > +// It has two uses in indep code, loading NaN, and loading 0.0 > +// loading 1.0, then subtracting from itself will almost certainly be faster. Why not make this function do just that for those special-cases?
Attachment #598836 -
Flags: review?(Jacob.Bramley) → review+
Reporter | ||
Comment 2•12 years ago
|
||
landed: http://hg.mozilla.org/projects/ionmonkey/rev/2c61c3d67b0f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•