Bug 167736 - [JSC] Add operationToInt32SensibleSlow to optimize kraken pbkdf2 and sha256
Summary: [JSC] Add operationToInt32SensibleSlow to optimize kraken pbkdf2 and sha256
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-02 09:05 PST by Yusuke Suzuki
Modified: 2017-02-04 13:22 PST (History)
9 users (show)

See Also:


Attachments
Patch (10.69 KB, patch)
2017-02-02 09:13 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (16.15 KB, patch)
2017-02-03 05:09 PST, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-02-02 09:05:20 PST
[JSC] Add operationToInt32Fallback to optimize kraken pbkdf2
Comment 1 Yusuke Suzuki 2017-02-02 09:13:11 PST
Created attachment 300414 [details]
Patch

WIP
Comment 2 Yusuke Suzuki 2017-02-02 09:13:44 PST
Still considering about the edge cases.
Comment 3 Yusuke Suzuki 2017-02-02 09:15:52 PST
CAUTION: Currently, this is only valid in x64 (sensible double to int32 case in DFG and FTL). And still I'm now considering about edge cases.
BTW, the latest ARM64 has super fast double to int32 operation. If this instruction becomes available, we can optimize this in ARM64.
Comment 4 Yusuke Suzuki 2017-02-02 09:34:39 PST
Pf course, later, we should make DFG prediction better not to emit ValueToInt32 in such a case.
But I think optimizing this sensitive double to int32 is worth doing.

Note that kraken pbkdf2 consumes 1/3 time for operationToInt32. ;)
Comment 5 Yusuke Suzuki 2017-02-02 10:13:08 PST
This also improves kraken sha256 iterative.
Comment 6 Yusuke Suzuki 2017-02-02 11:17:13 PST
Comment on attachment 300414 [details]
Patch

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

> Source/JavaScriptCore/runtime/MathCommon.cpp:481
> +    // If exponent < 0 there will be no bits to the left of the decimal point
> +    // after rounding; if the exponent is > 83 then no bits of precision can be
> +    // left in the low 32-bit range of the result (IEEE-754 doubles have 52 bits
> +    // of fractional precision).
> +    // Note this case handles 0, -0, and all infinite, NaN, & denormal value.
> +    if (exp < 0 || exp > 83)
> +        return 0;

And I think this check is also unnecessary.
1. If exp < 0, truncate should succeed.
2. If exp > 83 (exp >= 84), the following code produces 0 as a result. (exp > 52, so static_cast<int32_t>(bits << (84 - 52)) -> 0)
Comment 7 Yusuke Suzuki 2017-02-03 05:09:23 PST
Created attachment 300518 [details]
Patch
Comment 8 Saam Barati 2017-02-04 01:12:49 PST
Comment on attachment 300518 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:63
> +            1ff:   66 48 0f 7e c0          movq   %xmm0,%rax

I wonder why this doesn't do a mov rdx, rax

> Source/JavaScriptCore/runtime/MathCommon.cpp:470
> +int32_t JIT_OPERATION operationToInt32SensibleSlow(double number)

IEE754 is fun
Comment 9 Yusuke Suzuki 2017-02-04 05:46:14 PST
Comment on attachment 300518 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:63
>> +            1ff:   66 48 0f 7e c0          movq   %xmm0,%rax
> 
> I wonder why this doesn't do a mov rdx, rax

Yeah, that's storage. But it's due to the GCC6's decision.

>> Source/JavaScriptCore/runtime/MathCommon.cpp:470
>> +int32_t JIT_OPERATION operationToInt32SensibleSlow(double number)
> 
> IEE754 is fun

Yeah :)
Comment 10 Yusuke Suzuki 2017-02-04 05:47:51 PST
Committed r211670: <http://trac.webkit.org/changeset/211670>
Comment 11 Saam Barati 2017-02-04 13:16:37 PST
I've confirmed that this is a 1-1.5% progression on Mac Kraken.
Comment 12 Radar WebKit Bug Importer 2017-02-04 13:22:35 PST
<rdar://problem/30364482>