Bug 234408 – [JSC] OpPow should have a "small int exponent" fast path at lower tiers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[JSC] OpPow should have a "small int exponent" fast path at lower tiers

[Ross Kirsling]

Created attachment 447395 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 447395 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1120
> +    bigt t1, 1000, .slow

Let's define 1000 as a constant in C++. And use `constexpr XXX` here. And in power implementation, we should use that constant too.

[Ross Kirsling]

Created attachment 447406 [details]
Patch

[Ross Kirsling]

Created attachment 447418 [details]
Patch

[Ross Kirsling]

Created attachment 447425 [details]
Patch

[Ross Kirsling]

Comment on attachment 447425 [details]
Patch

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

A couple of notes.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:496
> +    addSlowCase(branch32(GreaterThan, rightRegs.payloadGPR(), TrustedImm32(maxExponentForIntegerMathPow)));

I do wonder if maxExponentForIntegerMathPow should be 1024 instead of 1000. It hardly matters but it seems weird to use a base-10 threshold for bitshifting.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:-964
> -/* ------------------------------ END: OP_ADD, OP_SUB, OP_MUL, OP_POW ------------------------------ */

(Removing this since it's an END with no BEGIN and this file has not had an OP_POW.)

[Ross Kirsling]

Comment on attachment 447425 [details]
Patch

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

A couple of notes.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:496
> +    addSlowCase(branch32(GreaterThan, rightRegs.payloadGPR(), TrustedImm32(maxExponentForIntegerMathPow)));

I do wonder if maxExponentForIntegerMathPow should be 1024 instead of 1000. It hardly matters but it seems weird to use a base-10 threshold for bitshifting.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:-964
> -/* ------------------------------ END: OP_ADD, OP_SUB, OP_MUL, OP_POW ------------------------------ */

(Removing this since it's an END with no BEGIN and this file has not had an OP_POW.)

[Yusuke Suzuki]

Comment on attachment 447425 [details]
Patch

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

r=me

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1341
> +    addq numberTag, t0

What is the purpose of this addition? t0 is already 1, so just ci2ds is fine (it is not boxed double etc.).

> JSTests/microbenchmarks/pow-double-int.js:7
> +for (let i = -50; i <= 50; i += 0.1)
> +    for (let j = 0; j <= 1000; j++) 
> +        exponentiate(i, j);

Can you add a test case getting Infinity, -Infinity, and NaN for double input?

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #8)
> > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1341
> > +    addq numberTag, t0
> 
> What is the purpose of this addition? t0 is already 1, so just ci2ds is fine
> (it is not boxed double etc.).

Thanks for pointing this out -- I assumed that I needed it in order to do a double calculation with an immediate, but apparently I was wrong.

> > JSTests/microbenchmarks/pow-double-int.js:7
> > +for (let i = -50; i <= 50; i += 0.1)
> > +    for (let j = 0; j <= 1000; j++) 
> > +        exponentiate(i, j);
> 
> Can you add a test case getting Infinity, -Infinity, and NaN for double
> input?

I think this is covered by the following:
stress/pow-nan-behaviors.js
stress/pow-with-constants.js

But let me know if we need more.

[Ross Kirsling]

Created attachment 447541 [details]
Patch for landing

[EWS]

Committed r287236 (245396@main): <https://commits.webkit.org/245396@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447541 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86680069>