Bug 47007 – ARMv7 JIT should take advantage of 2-byte branches to reduce code size

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Goodwin]

ARMv7 JIT currently uses only 4-byte versions of branch. It should also use the 2-byte version when target is in range.

[David Goodwin]

Created attachment 69500 [details]
Patch: Use 2-byte branches when possible in ARMv7 JIT.

[David Goodwin]

Patch was tested with run-javascriptcore-tests.

[WebKit Review Bot]

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.

[Oliver Hunt]

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

[David Goodwin]

Created attachment 69656 [details]
Updated patch addressing reviewer comments.

[Oliver Hunt]

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();

[David Goodwin]

Created attachment 69668 [details]
patch tweaks

[Oliver Hunt]

Comment on attachment 69668 [details]
patch tweaks

r=me, should be committed automatically in a wee while

[WebKit Commit Bot]

Comment on attachment 69668 [details]
patch tweaks

Clearing flags on attachment: 69668

Committed r69080: <http://trac.webkit.org/changeset/69080>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.