[webkit-reviews] review denied: [Bug 47007] ARMv7 JIT should take advantage of 2-byte branches to reduce code size : [Attachment 69500] Patch: Use 2-byte branches when possible in ARMv7 JIT.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 4 11:11:34 PDT 2010


Oliver Hunt <oliver at apple.com> has denied David Goodwin
<david_goodwin at apple.com>'s request for review:
Bug 47007: ARMv7 JIT should take advantage of 2-byte branches to reduce code
size
https://bugs.webkit.org/show_bug.cgi?id=47007

Attachment 69500: Patch: Use 2-byte branches when possible in ARMv7 JIT.
https://bugs.webkit.org/attachment.cgi?id=69500&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
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


More information about the webkit-reviews mailing list