Bug 32190

Summary: conway benchmark spends half it's time in op_less (jump fusion fails)
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: JavaScriptCoreAssignee: Maciej Stachowiak <mjs>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Fix style issues; remove bogus sunspider change; add tests for float case codegen oliver: review+

Maciej Stachowiak
Reported 2009-12-05 22:30:40 PST
The "conway" test on the new Celtic Kane benchmark spends half its time in op_less. This is because it does conditionals like this: if (a < b && c > d)... and the logical and operator prevents any branch/condition fusion from taking place.
Attachments
Patch (49.75 KB, patch)
2009-12-05 23:08 PST, Maciej Stachowiak
no flags
Patch (48.58 KB, patch)
2009-12-05 23:25 PST, Maciej Stachowiak
no flags
Fix style issues; remove bogus sunspider change; add tests for float case codegen (59.30 KB, patch)
2009-12-06 00:33 PST, Maciej Stachowiak
oliver: review+
Maciej Stachowiak
Comment 1 2009-12-05 23:08:41 PST
WebKit Review Bot
Comment 2 2009-12-05 23:11:59 PST
Attachment 44354 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 JavaScriptCore/jit/JITArithmetic.cpp:151: JIT::emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:188: JIT::emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1529: JIT::emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1567: JIT::emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1595: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/jit/JITArithmetic.cpp:1636: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/jit/JIT.h:739: emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:748: emit_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:823: emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:829: emitSlow_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITOpcodes.cpp:746: JIT::emit_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITOpcodes.cpp:777: JIT::emitSlow_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/interpreter/Interpreter.cpp:2650: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/interpreter/Interpreter.cpp:2833: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 14
Maciej Stachowiak
Comment 3 2009-12-05 23:25:47 PST
WebKit Review Bot
Comment 4 2009-12-05 23:27:33 PST
Attachment 44357 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 JavaScriptCore/jit/JITArithmetic.cpp:151: JIT::emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:188: JIT::emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1529: JIT::emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1567: JIT::emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1595: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/jit/JITArithmetic.cpp:1636: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/jit/JIT.h:739: emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:748: emit_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:823: emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:829: emitSlow_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITOpcodes.cpp:746: JIT::emit_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITOpcodes.cpp:777: JIT::emitSlow_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/interpreter/Interpreter.cpp:2650: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/interpreter/Interpreter.cpp:2833: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 14
Adam Barth
Comment 5 2009-12-05 23:36:59 PST
Are these naming warnings useful? Do we plan to change these names to use camel case?
Oliver Hunt
Comment 6 2009-12-05 23:49:38 PST
Comment on attachment 44357 [details] Patch r=me on the code, though i'd like gbarra or ggaren to also look at the jit code gen. The changes to the sunspider script seem accidental I'd prefer it if the test case also covered float values in addition to integers. --Oliver
Maciej Stachowiak
Comment 7 2009-12-06 00:03:00 PST
(In reply to comment #5) > Are these naming warnings useful? Do we plan to change these names to use > camel case? The opcode-related names are an exception to the usual naming rules. They should probably be whitelisted somehow for style checking purposes. Allowing names with _op_ in the JavaScriptCore directory would probably do it.
Maciej Stachowiak
Comment 8 2009-12-06 00:13:57 PST
(In reply to comment #4) > > JavaScriptCore/interpreter/Interpreter.cpp:2650: Place brace on its own line > for function definitions. [whitespace/braces] [4] > JavaScriptCore/interpreter/Interpreter.cpp:2833: Place brace on its own line > for function definitions. [whitespace/braces] [4] These are also false positives (the lines in question are funky funky macros followed by a brace, not function definitions).
Maciej Stachowiak
Comment 9 2009-12-06 00:33:52 PST
Created attachment 44358 [details] Fix style issues; remove bogus sunspider change; add tests for float case codegen
WebKit Review Bot
Comment 10 2009-12-06 00:38:39 PST
Attachment 44358 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 JavaScriptCore/jit/JITArithmetic.cpp:151: JIT::emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:188: JIT::emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1529: JIT::emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITArithmetic.cpp:1567: JIT::emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:739: emit_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:748: emit_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:823: emitSlow_op_jless is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JIT.h:829: emitSlow_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITOpcodes.cpp:746: JIT::emit_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/jit/JITOpcodes.cpp:777: JIT::emitSlow_op_loop_if_false is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/interpreter/Interpreter.cpp:2650: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/interpreter/Interpreter.cpp:2833: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12
Oliver Hunt
Comment 11 2009-12-06 01:19:19 PST
Comment on attachment 44358 [details] Fix style issues; remove bogus sunspider change; add tests for float case codegen r=me
Maciej Stachowiak
Comment 12 2009-12-06 01:42:30 PST
Adam Barth
Comment 13 2009-12-06 08:44:30 PST
> The opcode-related names are an exception to the usual naming rules. They > should probably be whitelisted somehow for style checking purposes. Allowing > names with _op_ in the JavaScriptCore directory would probably do it. Filed https://bugs.webkit.org/show_bug.cgi?id=32192
Adam Barth
Comment 14 2009-12-06 08:48:16 PST
> These are also false positives (the lines in question are funky funky macros > followed by a brace, not function definitions). Filed: https://bugs.webkit.org/show_bug.cgi?id=32193
Note You need to log in before you can comment on or make changes to this bug.