Summary: | We should have SSE4 detection in the X86 MacroAssembler. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, jfbastien, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Keith Miller
2016-12-03 13:29:56 PST
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> |