Bug 88314 - Improve Math.round and Math.floor intrinsic
Summary: Improve Math.round and Math.floor intrinsic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-05 01:38 PDT by Yuqiang Xian
Modified: 2022-02-27 23:24 PST (History)
2 users (show)

See Also:


Attachments
patch (5.18 KB, patch)
2012-06-05 01:52 PDT, Yuqiang Xian
fpizlo: review+
Details | Formatted Diff | Diff
Performance result (6.94 KB, text/plain)
2012-06-05 01:55 PDT, Yuqiang Xian
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yuqiang Xian 2012-06-05 01:38:37 PDT
Currently we call a native function from the JIT code to complete the "round" and "floor" operations. We could inline some fast paths especially for those positive values on the platforms where floating point truncation is supported.
Comment 1 Yuqiang Xian 2012-06-05 01:52:18 PDT
Created attachment 145729 [details]
patch
Comment 2 Yuqiang Xian 2012-06-05 01:55:00 PDT
Created attachment 145730 [details]
Performance result

~3% improvement on Kraken (32% on audio-oscillator), and nearly 1% on SunSpider.
Comment 3 Yuqiang Xian 2012-06-05 01:58:30 PDT
(In reply to comment #2)
> Created an attachment (id=145730) [details]
> Performance result
> 
> ~3% improvement on Kraken (32% on audio-oscillator), and nearly 1% on SunSpider.

o... for clarification, overall SS improvement is not obvious, but definitely 9% on one case.
Comment 4 Filip Pizlo 2012-06-05 20:47:07 PDT
Comment on attachment 145729 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145729&action=review

This looks right.  I'm curious what your thoughts are about the comparing against neg zero.  Maybe this code would work the same if you just used zero instead of neg zero, which would be faster, since regular zero is easier to load in SSE?

Not marking cq+ just because I want to give you an opportunity to respond.  But I think the code is correct so if there's nothing that needs changing then I'm fine with this being landed as-is.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:197
> +        doubleResult.append(jit.branchDouble(MacroAssembler::DoubleEqual, SpecializedThunkJIT::fpRegT0, SpecializedThunkJIT::fpRegT1));

Is this right?  Double equality has strange handling of zero.  0 == -0 for example.
Comment 5 Yuqiang Xian 2012-06-05 21:02:17 PDT
(In reply to comment #4)
> (From update of attachment 145729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145729&action=review
> 
> This looks right.  I'm curious what your thoughts are about the comparing against neg zero.  Maybe this code would work the same if you just used zero instead of neg zero, which would be faster, since regular zero is easier to load in SSE?
> 
> Not marking cq+ just because I want to give you an opportunity to respond.  But I think the code is correct so if there's nothing that needs changing then I'm fine with this being landed as-is.
> 
> > Source/JavaScriptCore/jit/ThunkGenerators.cpp:197
> > +        doubleResult.append(jit.branchDouble(MacroAssembler::DoubleEqual, SpecializedThunkJIT::fpRegT0, SpecializedThunkJIT::fpRegT1));
> 
> Is this right?  Double equality has strange handling of zero.  0 == -0 for example.

You're right. It should be 0.
Comment 6 Yuqiang Xian 2012-06-05 22:28:24 PDT
Landed with Filip's comments addressed: http://trac.webkit.org/changeset/119558