Bug 233275 - [JSC] Add branchTest16 operation
Summary: [JSC] Add branchTest16 operation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 233274
  Show dependency tree / graph
 
Reported: 2021-11-17 12:36 PST by Dean Jackson
Modified: 2021-11-18 12:05 PST (History)
9 users (show)

See Also:

Attachments
Patch (15.26 KB, patch)
2021-11-17 13:29 PST, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.13 KB, patch)
2021-11-17 13:49 PST, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.79 KB, patch)
2021-11-17 14:03 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (26.20 KB, patch)
2021-11-17 16:38 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (27.22 KB, patch)
2021-11-17 16:50 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (28.18 KB, patch)
2021-11-17 17:17 PST, Yusuke Suzuki 		mark.lam: review+
Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2021-11-17 12:36:08 PST 
For https://bugs.webkit.org/show_bug.cgi?id=233274, we need a branchTest16 to compare a raw OptionSet<uint16_t> value.
Comment 1 Darin Adler 2021-11-17 13:02:02 PST 
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.
Comment 2 Yusuke Suzuki 2021-11-17 13:29:55 PST 
Created attachment 444563 [details]
Patch
Comment 3 Yusuke Suzuki 2021-11-17 13:49:30 PST 
Created attachment 444566 [details]
Patch
Comment 4 Yusuke Suzuki 2021-11-17 14:03:57 PST 
Created attachment 444569 [details]
Patch
Comment 5 Yusuke Suzuki 2021-11-17 16:38:39 PST 
Created attachment 444602 [details]
Patch
Comment 6 Yusuke Suzuki 2021-11-17 16:50:46 PST 
Created attachment 444608 [details]
Patch
Comment 7 Yusuke Suzuki 2021-11-17 17:17:14 PST 
Created attachment 444614 [details]
Patch
Comment 8 Mark Lam 2021-11-18 11:56:06 PST 
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.
Comment 9 Yusuke Suzuki 2021-11-18 12:03:48 PST 
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 :)
Comment 10 Yusuke Suzuki 2021-11-18 12:04:59 PST 
Committed r286020 (244409@main): <https://commits.webkit.org/244409@main>
Comment 11 Radar WebKit Bug Importer 2021-11-18 12:05:26 PST 
<rdar://problem/85562051>