RESOLVED FIXED 47384
ARMv7 JIT should generated conditional branches when possible
https://bugs.webkit.org/show_bug.cgi?id=47384
Summary ARMv7 JIT should generated conditional branches when possible
David Goodwin
Reported 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.
Attachments
patch to use conditional branches when possible (29.92 KB, patch)
2010-10-07 16:40 PDT, David Goodwin
oliver: review-
Add more descriptive ChangeLog entry (30.59 KB, patch)
2010-10-11 10:19 PDT, David Goodwin
no flags
more descriptive ChangeLog (now without tabs, sigh) (30.65 KB, patch)
2010-10-11 10:26 PDT, David Goodwin
no flags
Patch (31.21 KB, patch)
2010-10-13 15:25 PDT, David Goodwin
no flags
David Goodwin
Comment 1 2010-10-07 16:40:12 PDT
Created attachment 70170 [details] patch to use conditional branches when possible
WebKit Review Bot
Comment 2 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.
Oliver Hunt
Comment 3 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.
David Goodwin
Comment 4 2010-10-11 10:19:13 PDT
Created attachment 70443 [details] Add more descriptive ChangeLog entry
WebKit Review Bot
Comment 5 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.
David Goodwin
Comment 6 2010-10-11 10:26:08 PDT
Created attachment 70449 [details] more descriptive ChangeLog (now without tabs, sigh)
WebKit Review Bot
Comment 7 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.
Oliver Hunt
Comment 8 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?
David Goodwin
Comment 9 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.
Geoffrey Garen
Comment 10 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.
Oliver Hunt
Comment 11 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.
David Goodwin
Comment 12 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.
Oliver Hunt
Comment 13 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?
David Goodwin
Comment 14 2010-10-13 15:25:03 PDT
Oliver Hunt
Comment 15 2010-10-13 15:27:14 PDT
Comment on attachment 70672 [details] Patch r=me!
WebKit Review Bot
Comment 16 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.
WebKit Commit Bot
Comment 17 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
Oliver Hunt
Comment 18 2010-10-13 19:52:56 PDT
Comment on attachment 70672 [details] Patch /me stabs commit queue and tries again
Eric Seidel (no email)
Comment 19 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.
WebKit Commit Bot
Comment 20 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>
WebKit Commit Bot
Comment 21 2010-10-13 23:45:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.