Bug 226266

Summary:

Add LLInt fast path for less, lesseq, greater and greatereq

Product:

WebKit

Reporter:

Mikhail R. Gadelha <mikhail>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

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

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Mikhail R. Gadelha

Reported 2021-05-26 07:12:12 PDT

Add LLInt fast path for less, lesseq, greater and greatereq

Attachments
Patch (5.94 KB, patch) 2021-05-26 07:13 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (7.26 KB, patch) 2021-05-27 14:36 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (8.74 KB, patch) 2021-05-28 11:46 PDT, Mikhail R. Gadelha	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (10.48 KB, patch) 2021-05-28 12:06 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (10.62 KB, patch) 2021-05-28 13:33 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (10.44 KB, patch) 2021-06-02 18:04 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (10.44 KB, patch) 2021-06-03 08:55 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (10.44 KB, patch) 2021-06-04 13:37 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (10.57 KB, patch) 2021-06-06 12:39 PDT, Mikhail R. Gadelha	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (10.56 KB, patch) 2021-06-06 13:06 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (10.45 KB, patch) 2021-06-07 08:55 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (10.45 KB, patch) 2021-06-07 11:14 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (11.79 KB, patch) 2021-06-08 10:30 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (13.02 KB, patch) 2021-06-10 08:49 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (13.02 KB, patch) 2021-06-10 10:07 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (12.42 KB, patch) 2021-06-15 08:24 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Patch (12.40 KB, patch) 2021-06-25 06:38 PDT, Mikhail R. Gadelha	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Mikhail R. Gadelha

Comment 1 2021-05-26 07:13:07 PDT

Created attachment 429753 [details] Patch

Mikhail R. Gadelha

Comment 2 2021-05-27 14:36:52 PDT

Created attachment 429929 [details] Patch

Mikhail R. Gadelha

Comment 3 2021-05-28 11:46:37 PDT

Created attachment 430035 [details] Patch

Mikhail R. Gadelha

Comment 4 2021-05-28 12:06:39 PDT

Created attachment 430038 [details] Patch

Mikhail R. Gadelha

Comment 5 2021-05-28 13:33:03 PDT

Created attachment 430047 [details] Patch

Radar WebKit Bug Importer

Comment 6 2021-06-02 07:13:25 PDT

<rdar://problem/78761613>

Mikhail R. Gadelha

Comment 7 2021-06-02 18:04:14 PDT

Created attachment 430427 [details] Patch

Tadeu Zagallo

Comment 8 2021-06-02 18:13:37 PDT

Comment on attachment 430427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430427&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2003 > + ci2ds t1, ft0 nit: no reason to convert t1 to a float before branching. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2194 > + ci2ds t0, ft0 same nit here. I saw we do this in other places, so it's up to you if you want to move it.

Mikhail R. Gadelha

Comment 9 2021-06-03 08:55:30 PDT

Created attachment 430474 [details] Patch

Mikhail R. Gadelha

Comment 10 2021-06-03 08:57:50 PDT

Thanks for the review, Tadeu! I just sent a patch to see if ews is back and included your suggestion. Later I'll update the patch with some microbenchmarks I created.

Mikhail R. Gadelha

Comment 11 2021-06-04 13:37:06 PDT

Created attachment 430608 [details] Patch

Mikhail R. Gadelha

Comment 12 2021-06-06 12:39:39 PDT

Created attachment 430696 [details] Patch

Mikhail R. Gadelha

Comment 13 2021-06-06 13:06:47 PDT

Created attachment 430697 [details] Patch

Mikhail R. Gadelha

Comment 14 2021-06-07 08:55:52 PDT

Created attachment 430752 [details] Patch

Mikhail R. Gadelha

Comment 15 2021-06-07 11:14:57 PDT

Created attachment 430763 [details] Patch

Mikhail R. Gadelha

Comment 16 2021-06-08 10:30:02 PDT

Created attachment 430859 [details] Patch

Mikhail R. Gadelha

Comment 17 2021-06-09 08:08:14 PDT

On x86_64: baseline changes number-comparison-inline 12.2677+-0.0568 ^ 9.0738+-0.1066 ^ definitely 1.3520x faster <geometric> 12.2677+-0.0568 ^ 9.0738+-0.1066 ^ definitely 1.3520x faster On ARMv7: number-comparison-inline 58.4234+-1.0436 ^ 48.7810+-0.3148 ^ definitely 1.1977x faster <geometric> 58.4234+-1.0436 ^ 48.7810+-0.3148 ^ definitely 1.1977x faster

Caio Lima

Comment 18 2021-06-09 10:29:11 PDT

Comment on attachment 430427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430427&action=review > Source/JavaScriptCore/ChangeLog:7 > + We are missing CL description for this patch. We should explain here the motivation why we are doing this change and explain changes a bit as well. In addition, It would quite interesting getting documented here the results for running at least JetStream 2, for LLInt-only (You can use `PerformanceTest/JetStream2/watch-cli.js` for it) and results with full JIT support.

Mikhail R. Gadelha

Comment 19 2021-06-10 08:49:08 PDT

Created attachment 431083 [details] Patch

Mikhail R. Gadelha

Comment 20 2021-06-10 10:07:19 PDT

Created attachment 431090 [details] Patch

Mikhail R. Gadelha

Comment 21 2021-06-10 10:08:35 PDT

JetStream2 results: x86_64 jit: a mean = 286.20032 b mean = 290.47749 pValue = 0.3039127831 (Bigger means are better.) 1.015 times better Results ARE NOT significant x86_64 no-jit: a mean = 38.51948 b mean = 39.22851 pValue = 0.0876327821 (Bigger means are better.) 1.018 times better Results ARE NOT significant ARMv7 no-jit: a mean = 10.33014 b mean = 10.29042 pValue = 0.2110898172 (Bigger means are better.) 1.004 times worse Results ARE NOT significant

Caio Lima

Comment 22 2021-06-15 06:28:34 PDT

Comment on attachment 431090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431090&action=review LGTM as well. Since Tadeu gave you a r+ already, following patches you don’t need to set “r?" again, only if you made a major changes from previous patch. Ping me when you think patch is ready to get cq+. > Source/JavaScriptCore/ChangeLog:9 > + in LLInt, so we donât need to go to slow path for those cases. nit: there’s a strange character here. > Source/JavaScriptCore/ChangeLog:12 > + instruction for ARMv7, MIPS and CLoop. Could you include the numbers of benchmarks here? Also, it’s important to highlight that those numbers are from a LLInt-only configuration.

Mikhail R. Gadelha

Comment 23 2021-06-15 08:24:09 PDT

Created attachment 431440 [details] Patch

Mikhail R. Gadelha

Comment 24 2021-06-25 06:38:14 PDT

Created attachment 432260 [details] Patch

Caio Lima

Comment 25 2021-06-28 10:35:13 PDT

Comment on attachment 432260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432260&action=review > Source/JavaScriptCore/ChangeLog:21 > + * ARMv7 no-jit: 1.004 times worse I think those results were neutral, right?

Mikhail R. Gadelha

Comment 26 2021-06-28 10:50:04 PDT

Yeah, 0.4% is probably noise.

EWS

Comment 27 2021-06-28 10:55:21 PDT

Committed r279343 (239211@main): <https://commits.webkit.org/239211@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432260 [details].

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