Bug 47384 - ARMv7 JIT should generated conditional branches when possible
Summary: ARMv7 JIT should generated conditional branches when possible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-07 16:34 PDT by David Goodwin
Modified: 2010-10-13 23:45 PDT (History)
5 users (show)

See Also:


Attachments
patch to use conditional branches when possible (29.92 KB, patch)
2010-10-07 16:40 PDT, David Goodwin
oliver: review-
Details | Formatted Diff | Diff
Add more descriptive ChangeLog entry (30.59 KB, patch)
2010-10-11 10:19 PDT, David Goodwin
no flags Details | Formatted Diff | Diff
more descriptive ChangeLog (now without tabs, sigh) (30.65 KB, patch)
2010-10-11 10:26 PDT, David Goodwin
no flags Details | Formatted Diff | Diff
Patch (31.21 KB, patch)
2010-10-13 15:25 PDT, David Goodwin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Goodwin 2010-10-07 16:34:19 PDT
Current, ARMv7 JIT generates a conditional branch using IT with an unconditional branch. The JIT should instead use a conditional branch when possible.
Comment 1 David Goodwin 2010-10-07 16:40:12 PDT
Created attachment 70170 [details]
patch to use conditional branches when possible
Comment 2 WebKit Review Bot 2010-10-07 16:46:00 PDT
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.
Comment 3 Oliver Hunt 2010-10-08 18:10:22 PDT
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.
Comment 4 David Goodwin 2010-10-11 10:19:13 PDT
Created attachment 70443 [details]
Add more descriptive ChangeLog entry
Comment 5 WebKit Review Bot 2010-10-11 10:21:09 PDT
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.
Comment 6 David Goodwin 2010-10-11 10:26:08 PDT
Created attachment 70449 [details]
more descriptive ChangeLog (now without tabs, sigh)
Comment 7 WebKit Review Bot 2010-10-11 10:28:51 PDT
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.
Comment 8 Oliver Hunt 2010-10-11 12:59:14 PDT
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?
Comment 9 David Goodwin 2010-10-13 08:51:16 PDT
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.
Comment 10 Geoffrey Garen 2010-10-13 11:23:41 PDT
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.
Comment 11 Oliver Hunt 2010-10-13 13:16:09 PDT
(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.
Comment 12 David Goodwin 2010-10-13 14:14:51 PDT
(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.
Comment 13 Oliver Hunt 2010-10-13 14:20:46 PDT
(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?
Comment 14 David Goodwin 2010-10-13 15:25:03 PDT
Created attachment 70672 [details]
Patch
Comment 15 Oliver Hunt 2010-10-13 15:27:14 PDT
Comment on attachment 70672 [details]
Patch

r=me!
Comment 16 WebKit Review Bot 2010-10-13 15:28:06 PDT
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.
Comment 17 WebKit Commit Bot 2010-10-13 18:22:21 PDT
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
Comment 18 Oliver Hunt 2010-10-13 19:52:56 PDT
Comment on attachment 70672 [details]
Patch

/me stabs commit queue and tries again
Comment 19 Eric Seidel (no email) 2010-10-13 20:14:51 PDT
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.
Comment 20 WebKit Commit Bot 2010-10-13 23:45:35 PDT
Comment on attachment 70672 [details]
Patch

Clearing flags on attachment: 70672

Committed r69743: <http://trac.webkit.org/changeset/69743>
Comment 21 WebKit Commit Bot 2010-10-13 23:45:42 PDT
All reviewed patches have been landed.  Closing bug.