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+

Description Maciej Stachowiak 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.
Comment 1 Maciej Stachowiak 2009-12-05 23:08:41 PST 
Created attachment 44354 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Maciej Stachowiak 2009-12-05 23:25:47 PST 
Created attachment 44357 [details]
Patch
Comment 4 WebKit Review Bot 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
Comment 5 Adam Barth 2009-12-05 23:36:59 PST 
Are these naming warnings useful?  Do we plan to change these names to use camel case?
Comment 6 Oliver Hunt 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
Comment 7 Maciej Stachowiak 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.
Comment 8 Maciej Stachowiak 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).
Comment 9 Maciej Stachowiak 2009-12-06 00:33:52 PST 
Created attachment 44358 [details]
Fix style issues; remove bogus sunspider change; add tests for float case codegen
Comment 10 WebKit Review Bot 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
Comment 11 Oliver Hunt 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
Comment 12 Maciej Stachowiak 2009-12-06 01:42:30 PST 
Committed r51735: <http://trac.webkit.org/changeset/51735>
Comment 13 Adam Barth 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
Comment 14 Adam Barth 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