Bug 165363 – We should have SSE4 detection in the X86 MacroAssembler.

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

[Keith Miller]

There are some instructions in SSE4 that would be nice to use. For example the Popcnt instruction is used by Wasm.

[Yusuke Suzuki]

Created attachment 336494 [details]
Patch

[EWS Watchlist]

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.

[JF Bastien]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 336526 [details]
Patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

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

[Yusuke Suzuki]

Committed r229988: <https://trac.webkit.org/changeset/229988>

[Radar WebKit Bug Importer]

<rdar://problem/38881415>