ARMv7 JIT currently uses only 4-byte versions of branch. It should also use the 2-byte version when target is in range.
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.