Bug 32190 - conway benchmark spends half it's time in op_less (jump fusion fails)
Summary: conway benchmark spends half it's time in op_less (jump fusion fails)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-05 22:30 PST by Maciej Stachowiak
Modified: 2009-12-06 08:48 PST (History)
2 users (show)

See Also:


Attachments
Patch (49.75 KB, patch)
2009-12-05 23:08 PST, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
Patch (48.58 KB, patch)
2009-12-05 23:25 PST, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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