Bug 32190 – conway benchmark spends half it's time in op_less (jump fusion fails)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

Created attachment 44354 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 44357 [details]
Patch

[WebKit Review Bot]

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]

Are these naming warnings useful?  Do we plan to change these names to use camel case?

[Oliver Hunt]

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]

(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]

(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]

Created attachment 44358 [details]
Fix style issues; remove bogus sunspider change; add tests for float case codegen

[WebKit Review Bot]

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 on attachment 44358 [details]
Fix style issues; remove bogus sunspider change; add tests for float case codegen

r=me

[Maciej Stachowiak]

Committed r51735: <http://trac.webkit.org/changeset/51735>

[Adam Barth]

> 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]

> 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