Current, ARMv7 JIT generates a conditional branch using IT with an unconditional branch. The JIT should instead use a conditional branch when possible.
Created attachment 70170 [details] patch to use conditional branches when possible
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 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.
Created attachment 70443 [details] Add more descriptive ChangeLog entry
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.
Created attachment 70449 [details] more descriptive ChangeLog (now without tabs, sigh)
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 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?
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.
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.
(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.
(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.
(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?
Created attachment 70672 [details] Patch
Comment on attachment 70672 [details] Patch r=me!
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 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 on attachment 70672 [details] Patch /me stabs commit queue and tries again
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 on attachment 70672 [details] Patch Clearing flags on attachment: 70672 Committed r69743: <http://trac.webkit.org/changeset/69743>
All reviewed patches have been landed. Closing bug.