Bug 165363 - We should have SSE4 detection in the X86 MacroAssembler.
Summary: We should have SSE4 detection in the X86 MacroAssembler.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-03 13:29 PST by Keith Miller
Modified: 2018-03-26 14:08 PDT (History)
12 users (show)

See Also:


Attachments
Patch (20.12 KB, patch)
2018-03-25 05:26 PDT, Yusuke Suzuki
jfbastien: review+
Details | Formatted Diff | Diff
Patch for landing (20.35 KB, patch)
2018-03-26 11:06 PDT, Yusuke Suzuki
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-12-03 13:29:56 PST
There are some instructions in SSE4 that would be nice to use. For example the Popcnt instruction is used by Wasm.
Comment 1 Yusuke Suzuki 2018-03-25 05:26:36 PDT
Created attachment 336494 [details]
Patch
Comment 2 EWS Watchlist 2018-03-25 05:28:19 PDT
Attachment 336494 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:766:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:768:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:769:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:775:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:769:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:775:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:781:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:783:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:784:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:790:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:784:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:790:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:794:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 13 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 JF Bastien 2018-03-26 10:07:09 PDT
Comment on attachment 336494 [details]
Patch

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

r=me modulo some comments, and red bots.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:769
> +        : "0"(level)

I think for some you need to also set ECX=0 as input, so "c"(0) here?

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:804
> +            s_bmi1CheckState = (cpuid[2] & (1 << 3)) ? CPUIDCheckState::Set : CPUIDCheckState::Clear;

I think this is wrong, it's not under 0x80000001? It's EAX=7, ECX=0.

> Source/WTF/wtf/Atomics.h:-316
> -        "popl %%ebx\n\t"

This saved EBX but the code below doesn't. Why is that OK for x86-32?
Comment 4 Yusuke Suzuki 2018-03-26 11:02:46 PDT
Comment on attachment 336494 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:769
>> +        : "0"(level)
> 
> I think for some you need to also set ECX=0 as input, so "c"(0) here?

It's basically OK if we do not want extended values. But it would be nice. I'll just call getCPUIDEx(level, 0) here.

>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:804
>> +            s_bmi1CheckState = (cpuid[2] & (1 << 3)) ? CPUIDCheckState::Set : CPUIDCheckState::Clear;
> 
> I think this is wrong, it's not under 0x80000001? It's EAX=7, ECX=0.

Oh, nice. I copied the logic described before (in MSVC ifdef), but it was wrong. Fixed.

>> Source/WTF/wtf/Atomics.h:-316
>> -        "popl %%ebx\n\t"
> 
> This saved EBX but the code below doesn't. Why is that OK for x86-32?

In an ideal world, we can just specify `"=b"(xxx)` for asm template to indicate that this code can clobber ebx in this asm template.
However, in old GCC, we cannot use ebx for asm template constraint. So instead of specifying `"=b"(xxx)` constraint, we just say this code does not clobber ebx.
And we ensure that this code does not clobber ebx by manually pushing and popping it.

But now, our supporting GCC allows us to specify ebx for asm template's constraint. So this code is not necessary: just specifying "=b"(b) works.
Comment 5 Yusuke Suzuki 2018-03-26 11:06:53 PDT
Created attachment 336526 [details]
Patch for landing
Comment 6 EWS Watchlist 2018-03-26 11:09:28 PDT
Attachment 336526 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:775:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:777:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:778:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:784:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:778:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:784:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:788:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 7 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 WebKit Commit Bot 2018-03-26 11:41:59 PDT
Comment on attachment 336526 [details]
Patch for landing

Rejecting attachment 336526 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 336526, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/7105420
Comment 8 Yusuke Suzuki 2018-03-26 14:07:26 PDT
Committed r229988: <https://trac.webkit.org/changeset/229988>
Comment 9 Radar WebKit Bug Importer 2018-03-26 14:08:18 PDT
<rdar://problem/38881415>