[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