Bug 184023 - Emit fjcvtzs on ARM64E on Darwin
Summary: Emit fjcvtzs on ARM64E on Darwin
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: 185055
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-26 14:20 PDT by Yusuke Suzuki
Modified: 2018-10-17 12:20 PDT (History)
15 users (show)

See Also:


Attachments
WIP (8.06 KB, patch)
2018-10-14 00:19 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (10.62 KB, patch)
2018-10-14 14:49 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (12.87 KB, patch)
2018-10-14 14:53 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (14.33 KB, patch)
2018-10-14 15:01 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (14.12 KB, patch)
2018-10-14 15:02 PDT, Saam Barati
yusukesuzuki: review+
Details | Formatted Diff | Diff
patch for landing (14.16 KB, patch)
2018-10-15 11:35 PDT, 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 Yusuke Suzuki 2018-03-26 14:20:47 PDT
ARMv8.3-a adds a new instruction "jscvt", which can be used for converting double to int32_t in JS semantics.
Comment 1 Yusuke Suzuki 2018-03-26 14:56:18 PDT
In Linux, we can use hwcap to detect this feature. https://github.com/torvalds/linux/blob/3b3b681097fae73b7f5dcdd42db6cfdf32943d4c/Documentation/arm64/elf_hwcaps.txt
Comment 2 Yusuke Suzuki 2018-03-26 15:18:24 PDT
(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.
Comment 3 JF Bastien 2018-03-26 15:21:38 PDT
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.
Comment 4 Saam Barati 2018-10-08 00:33:50 PDT
Ima try to get to this this week
Comment 5 Yusuke Suzuki 2018-10-13 16:33:10 PDT
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.
Comment 6 Saam Barati 2018-10-14 00:19:10 PDT
Created attachment 352269 [details]
WIP
Comment 7 Build Bot 2018-10-14 00:21:40 PDT
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.
Comment 8 Saam Barati 2018-10-14 11:37:56 PDT
(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.
Comment 9 Saam Barati 2018-10-14 14:49:06 PDT
Created attachment 352277 [details]
WIP
Comment 10 Saam Barati 2018-10-14 14:53:09 PDT
Created attachment 352278 [details]
patch
Comment 11 Build Bot 2018-10-14 14:54:45 PDT
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.
Comment 12 Saam Barati 2018-10-14 15:01:04 PDT
Created attachment 352279 [details]
patch
Comment 13 Saam Barati 2018-10-14 15:02:06 PDT
Created attachment 352280 [details]
patch
Comment 14 Build Bot 2018-10-14 15:05:53 PDT
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.
Comment 15 Yusuke Suzuki 2018-10-15 07:09:07 PDT
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.
Comment 16 Filip Pizlo 2018-10-15 07:29:47 PDT
Comment on attachment 352280 [details]
patch

R=me too.
Comment 17 Mark Lam 2018-10-15 07:59:12 PDT
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.
Comment 18 Saam Barati 2018-10-15 11:08:40 PDT
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.
Comment 19 Saam Barati 2018-10-15 11:35:40 PDT
Created attachment 352349 [details]
patch for landing
Comment 20 Build Bot 2018-10-15 11:38:40 PDT
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.
Comment 21 WebKit Commit Bot 2018-10-15 12:47:19 PDT
Comment on attachment 352349 [details]
patch for landing

Clearing flags on attachment: 352349

Committed r237136: <https://trac.webkit.org/changeset/237136>
Comment 22 WebKit Commit Bot 2018-10-15 12:47:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2018-10-15 12:48:26 PDT
<rdar://problem/45281003>
Comment 24 Saam Barati 2018-10-17 11:28:00 PDT
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
Comment 25 Saam Barati 2018-10-17 11:48:13 PDT
Some devices show a 2% overall progression on JetStream 2. 

So it seems this is somewhere between a 0.5-2% progression.
Comment 26 Filip Pizlo 2018-10-17 12:20:24 PDT
(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!