Bug 184023 – Emit fjcvtzs on ARM64E on Darwin

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

ARMv8.3-a adds a new instruction "jscvt", which can be used for converting double to int32_t in JS semantics.

[Yusuke Suzuki]

In Linux, we can use hwcap to detect this feature. https://github.com/torvalds/linux/blob/3b3b681097fae73b7f5dcdd42db6cfdf32943d4c/Documentation/arm64/elf_hwcaps.txt

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #1)
> In Linux, we can use hwcap to detect this feature.
> https://github.com/torvalds/linux/blob/
> 3b3b681097fae73b7f5dcdd42db6cfdf32943d4c/Documentation/arm64/elf_hwcaps.txt

This is necessary since ARM64 feature registers are only available in privileged mode. To detect features, some kernel support is necessary. In Linux, we can use auxiliary vector provided by the kernel's ELF loader. This includes hwcaps, which tells us whether the CPU features are available.

[JF Bastien]

Asking the kernel is best.

If you can't you can also sigsetjmp, try the instruction, and then siglongjmp. It'll sigill if not supported. Caveat is that the kernel may decide to emulate instead of sigill, and then it'll be super slow.

[Saam Barati]

Ima try to get to this this week

[Yusuke Suzuki]

BTW, Linux MacroAssemblerARM64 already has "s_jscvtCheckState" state (runtime detection flag of jscvt availability).
If we emit jscvt based on this condition, we can support jscvt opcode on wider platforms.

[Saam Barati]

Created attachment 352269 [details]
WIP

[EWS Watchlist]

Attachment 352269 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MathCommon.h:144:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #5)
> BTW, Linux MacroAssemblerARM64 already has "s_jscvtCheckState" state
> (runtime detection flag of jscvt availability).
> If we emit jscvt based on this condition, we can support jscvt opcode on
> wider platforms.

Yeah makes sense. I saw that. I haven't gotten to integrating that yet.

[Saam Barati]

Created attachment 352277 [details]
WIP

[Saam Barati]

Created attachment 352278 [details]
patch

[EWS Watchlist]

Attachment 352278 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MathCommon.h:144:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 352279 [details]
patch

[Saam Barati]

Created attachment 352280 [details]
patch

[EWS Watchlist]

Attachment 352280 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MathCommon.h:144:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 352280 [details]
patch

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

r=me

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14440
> +#endif

Nice.

[Filip Pizlo]

Comment on attachment 352280 [details]
patch

R=me too.

[Mark Lam]

Comment on attachment 352280 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        This patch patch teaches JSC to use that instruction when possible.

duplicate "patch".

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:3764
> +#if HAVE(FJCVTZS_INSTRUCTION)

Should this be "HAVE(FJCVTZS_INSTRUCTION) && !OS(LINUX)"?  The code in MacroAssemblerARM64::collectCPUFeatures() implies that this is needed.

[Saam Barati]

Comment on attachment 352280 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        This patch patch teaches JSC to use that instruction when possible.
> 
> duplicate "patch".

Will fix.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:3764
>> +#if HAVE(FJCVTZS_INSTRUCTION)
> 
> Should this be "HAVE(FJCVTZS_INSTRUCTION) && !OS(LINUX)"?  The code in MacroAssemblerARM64::collectCPUFeatures() implies that this is needed.

HAVE(FJCVTZS) is only true on Darwin ATM. If it ever became true somewhere else at compile time, there’s be no need to make sure that we call that function on Linux, since it’s known based on the compile target that we’re going to have this instruction.

[Saam Barati]

Created attachment 352349 [details]
patch for landing

[EWS Watchlist]

Attachment 352349 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MathCommon.h:144:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 352349 [details]
patch for landing

Clearing flags on attachment: 352349

Committed r237136: <https://trac.webkit.org/changeset/237136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45281003>

[Saam Barati]

This is a 0.5-1% progression on JetStream2 on our bots on devices that support this instruction.

As Yusuke predicted, these JetStream 2 subtests are improved:
- 15% better on stanford-crypto-aes
- 30% better on stanford-crypto-pbkf2
- 97% better on stanford-crypto-sha256

[Saam Barati]

Some devices show a 2% overall progression on JetStream 2. 

So it seems this is somewhere between a 0.5-2% progression.

[Filip Pizlo]

(In reply to Saam Barati from comment #25)
> Some devices show a 2% overall progression on JetStream 2. 
> 
> So it seems this is somewhere between a 0.5-2% progression.

Nice!