Bug 183088 - Make Number.isInteger an intrinsic
Summary: Make Number.isInteger an intrinsic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-23 10:20 PST by Saam Barati
Modified: 2018-02-23 16:49 PST (History)
14 users (show)

See Also:


Attachments
patch (22.89 KB, patch)
2018-02-23 13:24 PST, Saam Barati
jfbastien: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
patch for landing (23.19 KB, patch)
2018-02-23 15:57 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-02-23 10:20:09 PST
This function was somewhat hot inside the ML subtest in ARES. Making it an intrinsic is not a perf improvement as far as I can measure, but I already did the work, so we might as well land it, since it is a perf improvement over calling the builtin function.
Comment 1 Saam Barati 2018-02-23 13:24:18 PST
Created attachment 334545 [details]
patch
Comment 2 JF Bastien 2018-02-23 13:37:21 PST
Comment on attachment 334545 [details]
patch

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

r=me, a few comments.

> JSTests/stress/number-is-integer-intrinsic.js:45
> +    [function(){}, false],

Add something with valueOf (returning both true and false things).

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4670
> +        auto notNanOrInfinity = m_jit.branch32(JITCompiler::NotEqual, TrustedImm32(0x7ff), resultGPR);

notNanNorInfinity?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4677
> +        m_jit.or32(TrustedImm32(ValueFalse), resultGPR);

Instead of having notNaNNorInfinity jump above, can you invert it, fall through where it should, and jump here if fails? Then you only OR in ValueFalse in one place (instead of move above, and moving ValueTrue below). I think it works if you set resultGPR early.
Comment 3 EWS Watchlist 2018-02-23 15:24:12 PST
Comment on attachment 334545 [details]
patch

Attachment 334545 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6645323

New failing tests:
stress/number-is-integer-intrinsic.js.ftl-eager-no-cjit
stress/number-is-integer-intrinsic.js.ftl-no-cjit-no-put-stack-validate
stress/number-is-integer-intrinsic.js.ftl-no-cjit-no-inline-validate
stress/number-is-integer-intrinsic.js.ftl-eager-no-cjit-b3o1
stress/number-is-integer-intrinsic.js.ftl-no-cjit-validate-sampling-profiler
Comment 4 Saam Barati 2018-02-23 15:26:36 PST
Comment on attachment 334545 [details]
patch

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

>> JSTests/stress/number-is-integer-intrinsic.js:45
>> +    [function(){}, false],
> 
> Add something with valueOf (returning both true and false things).

Thankfully, valueOf does not get called for isInteger.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4670
>> +        auto notNanOrInfinity = m_jit.branch32(JITCompiler::NotEqual, TrustedImm32(0x7ff), resultGPR);
> 
> notNanNorInfinity?

Yeah, will fix.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4677
>> +        m_jit.or32(TrustedImm32(ValueFalse), resultGPR);
> 
> Instead of having notNaNNorInfinity jump above, can you invert it, fall through where it should, and jump here if fails? Then you only OR in ValueFalse in one place (instead of move above, and moving ValueTrue below). I think it works if you set resultGPR early.

Yeah, this could work, but only if I do not mark result as "Reuse" above. The Reuse above makes it so that valueRegs.gpr() and resultGPR may be the same register. If they were, we couldn't do this. I went with trying to have less register pressure than this approach, but who knows if that's the better decision. I'm not really sure.
Comment 5 Saam Barati 2018-02-23 15:57:14 PST
Created attachment 334549 [details]
patch for landing
Comment 6 WebKit Commit Bot 2018-02-23 16:48:38 PST
Comment on attachment 334549 [details]
patch for landing

Clearing flags on attachment: 334549

Committed r228968: <https://trac.webkit.org/changeset/228968>
Comment 7 WebKit Commit Bot 2018-02-23 16:48:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-02-23 16:49:20 PST
<rdar://problem/37843990>