Bug 47007 - ARMv7 JIT should take advantage of 2-byte branches to reduce code size
Summary: ARMv7 JIT should take advantage of 2-byte branches to reduce code size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-01 12:15 PDT by David Goodwin
Modified: 2010-10-04 22:57 PDT (History)
4 users (show)

See Also:


Attachments
Patch: Use 2-byte branches when possible in ARMv7 JIT. (6.97 KB, patch)
2010-10-01 12:22 PDT, David Goodwin
oliver: review-
Details | Formatted Diff | Diff
Updated patch addressing reviewer comments. (9.83 KB, patch)
2010-10-04 11:54 PDT, David Goodwin
oliver: review-
Details | Formatted Diff | Diff
patch tweaks (9.84 KB, patch)
2010-10-04 12:24 PDT, David Goodwin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Goodwin 2010-10-01 12:15:53 PDT
ARMv7 JIT currently uses only 4-byte versions of branch. It should also use the 2-byte version when target is in range.
Comment 1 David Goodwin 2010-10-01 12:22:05 PDT
Created attachment 69500 [details]
Patch: Use 2-byte branches when possible in ARMv7 JIT.
Comment 2 David Goodwin 2010-10-01 12:22:55 PDT
Patch was tested with run-javascriptcore-tests.
Comment 3 WebKit Review Bot 2010-10-01 12:23:36 PDT
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 4 Oliver Hunt 2010-10-04 11:11:34 PDT
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
Comment 5 David Goodwin 2010-10-04 11:54:23 PDT
Created attachment 69656 [details]
Updated patch addressing reviewer comments.
Comment 6 Oliver Hunt 2010-10-04 11:58:45 PDT
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();
Comment 7 David Goodwin 2010-10-04 12:24:47 PDT
Created attachment 69668 [details]
patch tweaks
Comment 8 Oliver Hunt 2010-10-04 12:30:42 PDT
Comment on attachment 69668 [details]
patch tweaks

r=me, should be committed automatically in a wee while
Comment 9 WebKit Commit Bot 2010-10-04 22:57:04 PDT
Comment on attachment 69668 [details]
patch tweaks

Clearing flags on attachment: 69668

Committed r69080: <http://trac.webkit.org/changeset/69080>
Comment 10 WebKit Commit Bot 2010-10-04 22:57:09 PDT
All reviewed patches have been landed.  Closing bug.