Bug 204442 – [JSC] Extend MacroAssemblerARM64::load/store for datasize = 16

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Extend MacroAssemblerARM64::load/store for datasize = 16

[Yusuke Suzuki]

Created attachment 384026 [details]
Patch

WIP

[Yusuke Suzuki]

<rdar://problem/57366761>

[Yusuke Suzuki]

Created attachment 384027 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 384029 [details]
Patch

[Mark Lam]

Comment on attachment 384029 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        Our `void load16(const void* address, RegisterID dest)` and `void store16(RegisterID src, const void* address)` do not aware of

/do not aware/are not aware/.

> Source/JavaScriptCore/ChangeLog:10
> +        the condition that passed register is memoryTempRegister, while `MacroAssemblerARM64::{load,store}` handles it correctly, e.g.

/is memoryTempRegister/can be memoryTempRegister/

> Source/JavaScriptCore/ChangeLog:24
> +        to support 16 so that `or16` implementation looks like almost the same to `or32` etc.

/looks like almost the same to/is similar to/

> Source/JavaScriptCore/assembler/testmasm.cpp:1137
> +    // Just compiling test.
> +    compile([&] (CCallHelpers& jit) {
> +        emitFunctionPrologue(jit);
> +        jit.or16(CCallHelpers::TrustedImm32(42), CCallHelpers::AbsoluteAddress(nullptr));
> +        emitFunctionEpilogue(jit);
> +        jit.ret();
> +    });

How is this test case different than the one immediately above it?

[Yusuke Suzuki]

Comment on attachment 384029 [details]
Patch

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

>> Source/JavaScriptCore/assembler/testmasm.cpp:1137
>> +    });
> 
> How is this test case different than the one immediately above it?

nullptr is always invalid logical imm in ARM64, so this takes the path this patch is fixing.

[Mark Lam]

Comment on attachment 384029 [details]
Patch

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

r=me with fixes.

>>> Source/JavaScriptCore/assembler/testmasm.cpp:1137
>>> +    });
>> 
>> How is this test case different than the one immediately above it?
> 
> nullptr is always invalid logical imm in ARM64, so this takes the path this patch is fixing.

Yusuke told me offline that his intent is to take the !logicalImm.isValid() case in or16.  For that, we need to pass TrustedImm(0), not AbsoluteAddress(nullptr).

[Yusuke Suzuki]

Comment on attachment 384029 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:9
>> +        Our `void load16(const void* address, RegisterID dest)` and `void store16(RegisterID src, const void* address)` do not aware of
> 
> /do not aware/are not aware/.

Fixed.

>> Source/JavaScriptCore/ChangeLog:10
>> +        the condition that passed register is memoryTempRegister, while `MacroAssemblerARM64::{load,store}` handles it correctly, e.g.
> 
> /is memoryTempRegister/can be memoryTempRegister/

Fixed.

>> Source/JavaScriptCore/ChangeLog:24
>> +        to support 16 so that `or16` implementation looks like almost the same to `or32` etc.
> 
> /looks like almost the same to/is similar to/

Fixed.

>>>> Source/JavaScriptCore/assembler/testmasm.cpp:1137
>>>> +    });
>>> 
>>> How is this test case different than the one immediately above it?
>> 
>> nullptr is always invalid logical imm in ARM64, so this takes the path this patch is fixing.
> 
> Yusuke told me offline that his intent is to take the !logicalImm.isValid() case in or16.  For that, we need to pass TrustedImm(0), not AbsoluteAddress(nullptr).

Nice, fixed.

[Yusuke Suzuki]

Committed r252728: <https://trac.webkit.org/changeset/252728>