Bug 233275 – [JSC] Add branchTest16 operation

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

[Dean Jackson]

For https://bugs.webkit.org/show_bug.cgi?id=233274, we need a branchTest16 to compare a raw OptionSet<uint16_t> value.

[Darin Adler]

Even if we had no plans to switch to OptionSet, we need branchTest16 to check a bit in a uint16_t, rather than relying on the accident that branchTest32 happens to work for this on little-endian platforms.

I’m not sure that one patch needs to block the other. OptionSet would be no better or worse in this respect. But I agree we should do both things.

[Yusuke Suzuki]

Created attachment 444563 [details]
Patch

[Yusuke Suzuki]

Created attachment 444566 [details]
Patch

[Yusuke Suzuki]

Created attachment 444569 [details]
Patch

[Yusuke Suzuki]

Created attachment 444602 [details]
Patch

[Yusuke Suzuki]

Created attachment 444608 [details]
Patch

[Yusuke Suzuki]

Created attachment 444614 [details]
Patch

[Mark Lam]

Comment on attachment 444614 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1800
> +        // use addressTempRegister incase the branchTest16 we call uses dataTempRegister. :-/

/incase/in case/ and /branchTest16/branchTest32/

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1808
> +        // use addressTempRegister incase the branchTest16 we call uses dataTempRegister. :-/

Ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1816
> +        // use addressTempRegister incase the branchTest16 we call uses dataTempRegister. :-/

Ditto

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2102
> +        MacroAssemblerHelpers::load16OnCondition(*this, cond, address, dataTempRegister);
> +        return branchTest32(cond, dataTempRegister, mask16);

Should these be using addrTempRegister also?  Ditto below.

[Yusuke Suzuki]

Comment on attachment 444614 [details]
Patch

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

Thank you

>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1800
>> +        // use addressTempRegister incase the branchTest16 we call uses dataTempRegister. :-/
> 
> /incase/in case/ and /branchTest16/branchTest32/

Fixed.

>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1808
>> +        // use addressTempRegister incase the branchTest16 we call uses dataTempRegister. :-/
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1816
>> +        // use addressTempRegister incase the branchTest16 we call uses dataTempRegister. :-/
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2102
>> +        return branchTest32(cond, dataTempRegister, mask16);
> 
> Should these be using addrTempRegister also?  Ditto below.

MIPS is not using dataTempRegister in branchTest32 implementation so we can use it :)

[Yusuke Suzuki]

Committed r286020 (244409@main): <https://commits.webkit.org/244409@main>

[Radar WebKit Bug Importer]

<rdar://problem/85562051>