Bug 147761 - Add MacroAssembler::patchableBranch64 and fix ARM64's patchableBranchPtr
Summary: Add MacroAssembler::patchableBranch64 and fix ARM64's patchableBranchPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-06 18:08 PDT by Yusuke Suzuki
Modified: 2015-08-07 10:32 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.01 KB, patch)
2015-08-06 20:17 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-08-06 18:08:04 PDT
Yup. Currently, there's no patchableBranch64 helper function in MacroAssembler; it can reduce the following

cmp some condition
jump_ok pass
patchable jump
pass:

to

cmp some condition
patchable jump_ng

There's patchableBranchPtr in MacroAssembler under 64bit environments.
However, the current MacroAssemblerARM64's patchableBranchPtr seems wrong.

2344     PatchableJump patchableBranchPtr(RelationalCondition cond, Address left, TrustedImmPtr right = TrustedImmPtr(0))
2345     {
2346         m_makeJumpPatchable = true;
2347         Jump result = branch32(cond, left, TrustedImm32(right));
2348         m_makeJumpPatchable = false;
2349         return PatchableJump(result);
2350     }

It truncate the pointer to imm32. (And there's no patchableBranchPtr user in WebKit currently)
Comment 1 Yusuke Suzuki 2015-08-06 20:17:53 PDT
Created attachment 258437 [details]
Patch
Comment 2 Yusuke Suzuki 2015-08-06 20:22:59 PDT
The fix becomes pretty small :)
Comment 3 Mark Lam 2015-08-07 09:08:55 PDT
Comment on attachment 258437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258437&action=review

r=me

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:760
> +    PatchableJump patchableBranch64(RelationalCondition cond, RegisterID reg, TrustedImm64 imm = TrustedImm64(0))

I think the TrustedImm64 arg should not have a default value.  This conditional branch requires a comparison between 2 values.  It would read weird at the call site if of the values being compared is not specified.  Note: I see that compare64(), branch64(), and branchPtr() emitters all do not allow a default value for the argument as well.  The ARM64 version of this patchableBranch64() also do not.
Comment 4 Yusuke Suzuki 2015-08-07 10:28:16 PDT
Comment on attachment 258437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258437&action=review

Thank you for your review.

>> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:760
>> +    PatchableJump patchableBranch64(RelationalCondition cond, RegisterID reg, TrustedImm64 imm = TrustedImm64(0))
> 
> I think the TrustedImm64 arg should not have a default value.  This conditional branch requires a comparison between 2 values.  It would read weird at the call site if of the values being compared is not specified.  Note: I see that compare64(), branch64(), and branchPtr() emitters all do not allow a default value for the argument as well.  The ARM64 version of this patchableBranch64() also do not.

Make sense. I've dropped it. And I've dropped the default arg in ARM64 patchableBranchPtr as well.
Comment 5 Yusuke Suzuki 2015-08-07 10:32:00 PDT
Committed r188135: <http://trac.webkit.org/changeset/188135>