Summary: | ARMv7 JIT should take advantage of 2-byte branches to reduce code size | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Goodwin <david_goodwin> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ggaren, oliver, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | Other | ||||||||||
Attachments: |
|
Description
David Goodwin
2010-10-01 12:15:53 PDT
Created attachment 69500 [details]
Patch: Use 2-byte branches when possible in ARMv7 JIT.
Patch was tested with run-javascriptcore-tests. Attachment 69500 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/assembler/ARMv7Assembler.h:1738: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 69500 [details] Patch: Use 2-byte branches when possible in ARMv7 JIT. View in context: https://bugs.webkit.org/attachment.cgi?id=69500&action=review Patch basically looks good, we should just do that rename and fix the style issues before landing. > JavaScriptCore/assembler/ARMv7Assembler.h:2 > - * Copyright (C) 2009 Apple Inc. All rights reserved. > + * Copyright (C) 2010 Apple Inc. All rights reserved. I believe this change should be to 2009, 2010 > JavaScriptCore/assembler/ARMv7Assembler.h:483 > - enum JumpLinkType { LinkInvalid, LinkShortJump, LinkConditionalShortJump, LinkLongJump, JumpTypeCount }; > + enum JumpLinkType { LinkInvalid, LinkSmallestJump, LinkConditionalSmallestJump, > + LinkShortJump, LinkConditionalShortJump, LinkLongJump, JumpTypeCount }; These should be changes to be more technically specific -> LinkJumpT2, LinkConditionalJumpT2, LinkJumpT4, LinkConditionalJumpT4, LinkBX I think it makes sense to do that rename as part of this patch so we aren't having this Small vs. Smallest jump stuff which i suspect will only become more confusing in future > JavaScriptCore/assembler/ARMv7Assembler.h:1752 > + switch (linkType) { > + case LinkSmallestJump: > + case LinkConditionalSmallestJump: > + linkSmallestJump(reinterpret_cast<uint16_t*>(from), to); > + break; > + case LinkShortJump: > + case LinkConditionalShortJump: > + linkShortJump(reinterpret_cast<uint16_t*>(from), to); > + break; > + case LinkLongJump: > + linkLongJump(reinterpret_cast<uint16_t*>(from), to); > + break; > + default: > + ASSERT(false); > + break; > + } Webkit style is not to indent the case labels from the switch statement Created attachment 69656 [details]
Updated patch addressing reviewer comments.
Comment on attachment 69656 [details] Updated patch addressing reviewer comments. View in context: https://bugs.webkit.org/attachment.cgi?id=69656&action=review Just those two changes and then i'll r and cq+ this > JavaScriptCore/assembler/ARMv7Assembler.h:2 > + * Copyright (C) 2009,2010 Apple Inc. All rights reserved. should be a space here > JavaScriptCore/assembler/ARMv7Assembler.h:1750 > + ASSERT(false); This should be ASSERT_NOT_REACHED(); Created attachment 69668 [details]
patch tweaks
Comment on attachment 69668 [details]
patch tweaks
r=me, should be committed automatically in a wee while
Comment on attachment 69668 [details] patch tweaks Clearing flags on attachment: 69668 Committed r69080: <http://trac.webkit.org/changeset/69080> All reviewed patches have been landed. Closing bug. |