Bug 233275

Summary:

[JSC] Add branchTest16 operation

Product:

WebKit

Reporter:

Dean Jackson <dino>

Component:

JavaScriptCore

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

233274

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	none
Patch	mark.lam: review+

Dean Jackson

Reported 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.

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 attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 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.

Yusuke Suzuki

Comment 2 2021-11-17 13:29:55 PST

Created attachment 444563 [details] Patch

Yusuke Suzuki

Comment 3 2021-11-17 13:49:30 PST

Created attachment 444566 [details] Patch

Yusuke Suzuki

Comment 4 2021-11-17 14:03:57 PST

Created attachment 444569 [details] Patch

Yusuke Suzuki

Comment 5 2021-11-17 16:38:39 PST

Created attachment 444602 [details] Patch

Yusuke Suzuki

Comment 6 2021-11-17 16:50:46 PST

Created attachment 444608 [details] Patch

Yusuke Suzuki

Comment 7 2021-11-17 17:17:14 PST

Created attachment 444614 [details] Patch

Mark Lam

Comment 8 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.

Yusuke Suzuki

Comment 9 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 :)

Yusuke Suzuki

Comment 10 2021-11-18 12:04:59 PST

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

Radar WebKit Bug Importer

Comment 11 2021-11-18 12:05:26 PST

<rdar://problem/85562051>

Note You need to log in before you can comment on or make changes to this bug.