Bug 226266 – Add LLInt fast path for less, lesseq, greater and greatereq

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

[Mikhail R. Gadelha]

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

[Mikhail R. Gadelha]

Created attachment 429753 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 429929 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430035 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430038 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430047 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/78761613>

[Mikhail R. Gadelha]

Created attachment 430427 [details]
Patch

[Tadeu Zagallo]

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]

Created attachment 430474 [details]
Patch

[Mikhail R. Gadelha]

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]

Created attachment 430608 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430696 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430697 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430752 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430763 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430859 [details]
Patch

[Mikhail R. Gadelha]

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 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]

Created attachment 431083 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 431090 [details]
Patch

[Mikhail R. Gadelha]

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 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]

Created attachment 431440 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 432260 [details]
Patch

[Caio Lima]

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]

Yeah, 0.4% is probably noise.

[EWS]

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