There are some instructions in SSE4 that would be nice to use. For example the Popcnt instruction is used by Wasm.
Created attachment 336494 [details] Patch
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 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 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.
Created attachment 336526 [details] Patch for landing
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 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
Committed r229988: <https://trac.webkit.org/changeset/229988>
<rdar://problem/38881415>