Bug 47384 – ARMv7 JIT should generated conditional branches when possible

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Goodwin]

Current, ARMv7 JIT generates a conditional branch using IT with an unconditional branch. The JIT should instead use a conditional branch when possible.

[David Goodwin]

Created attachment 70170 [details]
patch to use conditional branches when possible

[WebKit Review Bot]

Attachment 70170 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/assembler/ARMv7Assembler.h:751:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:759:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:766:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:2161:  JUMP_TEMPORARY_REGISTER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 70170 [details]
patch to use conditional branches when possible

View in context: https://bugs.webkit.org/attachment.cgi?id=70170&action=review

You need a more detailed changelog but other than that this looks good

> JavaScriptCore/ChangeLog:7
> +

You should have a description of what you're changing in a patch this size.

[David Goodwin]

Created attachment 70443 [details]
Add more descriptive ChangeLog entry

[WebKit Review Bot]

Attachment 70443 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:751:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:759:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:766:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:2161:  JUMP_TEMPORARY_REGISTER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 14 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Goodwin]

Created attachment 70449 [details]
more descriptive ChangeLog (now without tabs, sigh)

[WebKit Review Bot]

Attachment 70449 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/assembler/ARMv7Assembler.h:751:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:759:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:766:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/ARMv7Assembler.h:2161:  JUMP_TEMPORARY_REGISTER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 70449 [details]
more descriptive ChangeLog (now without tabs, sigh)

View in context: https://bugs.webkit.org/attachment.cgi?id=70449&action=review

I'm not really comfortable with the new jumptype naming schem -- could you explain what patchable means?

you should also replace the a == 0 checks with !a

> JavaScriptCore/assembler/ARMv7Assembler.h:482
> +    enum JumpType { JumpFixed, JumpNoCondition, JumpCondition, PatchableJumpNoCondition, PatchableJumpCondition, JumpTypeCount };
> +    enum JumpLinkType { LinkInvalid, LinkJumpT1, LinkJumpT2, LinkJumpT3,

I don't really get this naming scheme -- what's meant by patchable vs. non patchable?

[David Goodwin]

A patchable jump is one that is not compressed because it may need to be patched after code generation. I'm happy to change the names to anything you think is more appropriate.

[Geoffrey Garen]

Here's an alternative:

1. JumpConditionFixedSize, JumpNoConditionFixedSize. "Fixed size" is a little more specific and modular, since you don't have to know that the patching machinery relies on fixed sized instructions in order to know what "fixed size" means. Using a suffix helps communicate that these are just like the other similarly named jumps, with the modifier of having a fixed size.

I think it would also be helpful to readability to add a "canCompact(JumpType)" helper function. That way, branch compacting code can just do this:

if (!canCompact(jumpType))
    // bail out

And if we add / remove jump types, we only need to modify the central helper function, not each of these clients. We can also place the comment explaining which jump types can't compact in one place.

[Oliver Hunt]

(In reply to comment #10)
> Here's an alternative:
> 
> 1. JumpConditionFixedSize, JumpNoConditionFixedSize. "Fixed size" is a little more specific and modular, since you don't have to know that the patching machinery relies on fixed sized instructions in order to know what "fixed size" means. Using a suffix helps communicate that these are just like the other similarly named jumps, with the modifier of having a fixed size.
> 
> I think it would also be helpful to readability to add a "canCompact(JumpType)" helper function. That way, branch compacting code can just do this:
> 
> if (!canCompact(jumpType))
>     // bail out
> 
> And if we add / remove jump types, we only need to modify the central helper function, not each of these clients. We can also place the comment explaining which jump types can't compact in one place.

These seem like good names, and canCompact is a good addition.  My primary issue with the Patchable prefixes were that I felt you could reasonably interpret "Patchable" as meaning the linker was able to patch the link into another form.

David you may find it helpful to run the style checker before you upload, it's in the normal checkout at:
WebKitTools/Scripts/check-webkit-style

There's also the webkit-patch script that can automate the process of submitting a patch if you're interested.

[David Goodwin]

(In reply to comment #11)
> David you may find it helpful to run the style checker before you upload, it's in the normal checkout at:
> WebKitTools/Scripts/check-webkit-style
> 
> There's also the webkit-patch script that can automate the process of submitting a patch if you're interested.

Yeah, I use that, but there are a lot of violations in the file already so it takes some work to weed out the new ones that I've introduced. Plus something like the JUMP_TEMPORARY_REGISTER I assume I can't fix because I am just using something that is already defined and in use elsewhere in the code.

[Oliver Hunt]

(In reply to comment #12)
> (In reply to comment #11)
> > David you may find it helpful to run the style checker before you upload, it's in the normal checkout at:
> > WebKitTools/Scripts/check-webkit-style
> > 
> > There's also the webkit-patch script that can automate the process of submitting a patch if you're interested.
> 
> Yeah, I use that, but there are a lot of violations in the file already so it takes some work to weed out the new ones that I've introduced. Plus something like the JUMP_TEMPORARY_REGISTER I assume I can't fix because I am just using something that is already defined and in use elsewhere in the code.

Ah i forgot it did that :-/

if you do:

webkit-patch post

Assuming you haven't made any changes to the header of the changelog (the bug url, etc) it should correctly post the changelog to the correct bug.  Before it does that though it will run check-webkit-style in such a way that it only complains about errors in the diff.

I wonder if check-webkit-style works "correctly" if you just run it on a diff?

[David Goodwin]

Created attachment 70672 [details]
Patch

[Oliver Hunt]

Comment on attachment 70672 [details]
Patch

r=me!

[WebKit Review Bot]

Attachment 70672 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/assembler/ARMv7Assembler.h:2169:  JUMP_TEMPORARY_REGISTER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 70672 [details]
Patch

Rejecting patch 70672 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ul.
Files=14, Tests=304,  1 wallclock secs ( 0.74 cusr +  0.17 csys =  0.91 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21548 test cases.
http/tests/incremental/frame-focus-before-load.html -> failed

Exiting early after 1 failures. 20655 tests run.
431.05s total testing time

20654 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
34 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4434017

[Oliver Hunt]

Comment on attachment 70672 [details]
Patch

/me stabs commit queue and tries again

[Eric Seidel (no email)]

You should really stab the flaky tests. :)  I'm working on making the commit-queue *even* less sensitive to flaky tests.  It requires tests to flake twice in a row to have such a failure currently.

[WebKit Commit Bot]

Comment on attachment 70672 [details]
Patch

Clearing flags on attachment: 70672

Committed r69743: <http://trac.webkit.org/changeset/69743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.