Bug 183088 – Make Number.isInteger an intrinsic

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

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.

[Saam Barati]

Created attachment 334545 [details]
patch

[JF Bastien]

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.

[EWS Watchlist]

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 334549 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 334549 [details]
patch for landing

Clearing flags on attachment: 334549

Committed r228968: <https://trac.webkit.org/changeset/228968>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37843990>