Bug 47384 - ARMv7 JIT should generated conditional branches when possible
: ARMv7 JIT should generated conditional branches when possible
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-10-07 16:34 PST by
Modified: 2010-10-13 23:45 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-10-07 16:34:19 PST
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 From 2010-10-07 16:40:12 PST -------
Created an attachment (id=70170) [details]
patch to use conditional branches when possible
------- Comment #2 From 2010-10-07 16:46:00 PST -------
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 From 2010-10-08 18:10:22 PST -------
(From update of attachment 70170 [details])
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 From 2010-10-11 10:19:13 PST -------
Created an attachment (id=70443) [details]
Add more descriptive ChangeLog entry
------- Comment #5 From 2010-10-11 10:21:09 PST -------
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 From 2010-10-11 10:26:08 PST -------
Created an attachment (id=70449) [details]
more descriptive ChangeLog (now without tabs, sigh)
------- Comment #7 From 2010-10-11 10:28:51 PST -------
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 From 2010-10-11 12:59:14 PST -------
(From update of attachment 70449 [details])
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 From 2010-10-13 08:51:16 PST -------
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 From 2010-10-13 11:23:41 PST -------
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 From 2010-10-13 13:16:09 PST -------
(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 From 2010-10-13 14:14:51 PST -------
(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 From 2010-10-13 14:20:46 PST -------
(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 From 2010-10-13 15:25:03 PST -------
Created an attachment (id=70672) [details]
Patch
------- Comment #15 From 2010-10-13 15:27:14 PST -------
(From update of attachment 70672 [details])
r=me!
------- Comment #16 From 2010-10-13 15:28:06 PST -------
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 From 2010-10-13 18:22:21 PST -------
(From update of attachment 70672 [details])
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 From 2010-10-13 19:52:56 PST -------
(From update of attachment 70672 [details])
/me stabs commit queue and tries again
------- Comment #19 From 2010-10-13 20:14:51 PST -------
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 From 2010-10-13 23:45:35 PST -------
(From update of attachment 70672 [details])
Clearing flags on attachment: 70672

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