Bug 159969 – [ES7] Introduce exponentiation expression

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

[Yusuke Suzuki]

[ES7] Introduce exponentiation expression (**)

[Yusuke Suzuki]

Created attachment 284109 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 284115 [details]
Patch

[Yusuke Suzuki]

Created attachment 284116 [details]
Patch

[Yusuke Suzuki]

Created attachment 284119 [details]
Patch

Drop unnecessary UNLIKELY and update the performance in ChangeLog

[Yusuke Suzuki]

Created attachment 284122 [details]
Patch

Fix ChangeLog

[Saam Barati]

Comment on attachment 284122 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/ChangeLog:28
> +            It means that `+x ** y` becomes a syntax error. This is intentional. Without a small font in a JS script,

did you mean to say "font" here?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3825
> +            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ArithPow, op1, op2));

What happens now if we use have objects for arguments to ArithPow?
Will we just repeatedly OSR exit? Can we fix that if it's the case? Maybe in a follow-up patch?

> Source/JavaScriptCore/parser/Parser.cpp:3274
> +        failIfTrue(match(POW) && isUnaryOpExcludingUpdateOp(leadingTokenTypeForUnaryExpression), "Expected a update expression on the left hand side of an exponentiation expression");

I don't think this error message is helpful.
Maybe we can say something about precedence and that the user should explicitly place parens to emphasize precedence.

> Source/JavaScriptCore/tests/stress/pow-basics.js:184
> +test6(-37.676522764377296, -1.0, -0.026541727490454296, -0.026541727490454296);

I think this should be test7

> Source/JavaScriptCore/tests/stress/pow-basics.js:210
> +test7(-37.676522764377296, 2.0, 1419.5203676146407, 1419.5203676146407);

I think this should be test8

> Source/JavaScriptCore/tests/stress/pow-basics.js:235
> +test8(37.676522764377296, 2.0, 1419.5203676146407, 1419.5203676146407);

I think this should be test9

> Source/JavaScriptCore/tests/stress/pow-basics.js:261
> +test9(-37.676522764377296, 3.0, -53482.591444930236, -53482.591444930236);

I think this should be test10

> Source/JavaScriptCore/tests/stress/pow-basics.js:286
> +test10(37.676522764377296, 3.0, 53482.591444930236, 53482.591444930236);

I think this should be test11

> Source/JavaScriptCore/tests/stress/pow-coherency.js:1
> +//@ skip

why?

> Source/JavaScriptCore/tests/stress/pow-nan-behaviors.js:1
> +// If y is NaN, the result is NaN.

It's worth saying in a comment you're talking about "x ** y"

> Source/JavaScriptCore/tests/stress/pow-nan-behaviors.js:75
> +// If y is â0, the result is 1, even if x is NaN.

Weird characters. Maybe not intended?

> Source/JavaScriptCore/tests/stress/pow-nan-behaviors.js:144
> +// If abs(x) is 1 and y is +â, the result is NaN.
> +// If abs(x) is 1 and y is ââ, the result is NaN.

Maybe it's wroth writing "Inf" instead of what I presume is an infinity unicode character

[Yusuke Suzuki]

Comment on attachment 284122 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:28
>> +            It means that `+x ** y` becomes a syntax error. This is intentional. Without a small font in a JS script,
> 
> did you mean to say "font" here?

Ah, I mean that -x**y
                    ^
                    if we can write this as superscript <- small font

Fixed the description.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3825
>> +            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ArithPow, op1, op2));
> 
> What happens now if we use have objects for arguments to ArithPow?
> Will we just repeatedly OSR exit? Can we fix that if it's the case? Maybe in a follow-up patch?

Yup! As the same to Math.pow(a, b) case, the OSR exit occurs.
I'll create the follow up patch supporting ArithPow(Untyped, Untyped). This is the consistent way to the other Arith nodes, like ArithMul, ArithSub etc.
In ArithSub, the same problem happens. And ArithSub avoids OSR exits for that by supporting ArithSub(Untyped, Untyped) (And DFG performs ToNumber operation).

I've filed it in https://bugs.webkit.org/show_bug.cgi?id=160012. And added FIXME here.

>> Source/JavaScriptCore/parser/Parser.cpp:3274
>> +        failIfTrue(match(POW) && isUnaryOpExcludingUpdateOp(leadingTokenTypeForUnaryExpression), "Expected a update expression on the left hand side of an exponentiation expression");
> 
> I don't think this error message is helpful.
> Maybe we can say something about precedence and that the user should explicitly place parens to emphasize precedence.

OK, I rephrase it,
"Unparenthesized unary expression found on the left hand side of an exponentiation expression"

>> Source/JavaScriptCore/tests/stress/pow-basics.js:184
>> +test6(-37.676522764377296, -1.0, -0.026541727490454296, -0.026541727490454296);
> 
> I think this should be test7

Ooops! Thanks. Fixed.

>> Source/JavaScriptCore/tests/stress/pow-basics.js:210
>> +test7(-37.676522764377296, 2.0, 1419.5203676146407, 1419.5203676146407);
> 
> I think this should be test8

Fixed.

>> Source/JavaScriptCore/tests/stress/pow-basics.js:235
>> +test8(37.676522764377296, 2.0, 1419.5203676146407, 1419.5203676146407);
> 
> I think this should be test9

Fixed.

>> Source/JavaScriptCore/tests/stress/pow-basics.js:261
>> +test9(-37.676522764377296, 3.0, -53482.591444930236, -53482.591444930236);
> 
> I think this should be test10

Fixed.

>> Source/JavaScriptCore/tests/stress/pow-basics.js:286
>> +test10(37.676522764377296, 3.0, 53482.591444930236, 53482.591444930236);
> 
> I think this should be test11

Fixed.

>> Source/JavaScriptCore/tests/stress/pow-coherency.js:1
>> +//@ skip
> 
> why?

This is the same to math-pow-coherency.js. Currently this test does not pass in GTK / EFL x86 32bit environment.
This is because operationMathPow cannot offer the consistent results to the JIT code due to x87.
I'm now handling this in https://bugs.webkit.org/show_bug.cgi?id=157787.

Note that Mac x86 32bit does not suffer from x87. This is becasue Mac's clang uses SSE by default, so x87 is not used.

>> Source/JavaScriptCore/tests/stress/pow-nan-behaviors.js:1
>> +// If y is NaN, the result is NaN.
> 
> It's worth saying in a comment you're talking about "x ** y"

Fixed.

>> Source/JavaScriptCore/tests/stress/pow-nan-behaviors.js:75
>> +// If y is â0, the result is 1, even if x is NaN.
> 
> Weird characters. Maybe not intended?

Oops, thanks. Changed it to -0.

>> Source/JavaScriptCore/tests/stress/pow-nan-behaviors.js:144
>> +// If abs(x) is 1 and y is ââ, the result is NaN.
> 
> Maybe it's wroth writing "Inf" instead of what I presume is an infinity unicode character

OK, fixed.

[Yusuke Suzuki]

Committed r203499: <http://trac.webkit.org/changeset/203499>

[Saam Barati]

Comment on attachment 284122 [details]
Patch

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

>>> Source/JavaScriptCore/parser/Parser.cpp:3274
>>> +        failIfTrue(match(POW) && isUnaryOpExcludingUpdateOp(leadingTokenTypeForUnaryExpression), "Expected a update expression on the left hand side of an exponentiation expression");
>> 
>> I don't think this error message is helpful.
>> Maybe we can say something about precedence and that the user should explicitly place parens to emphasize precedence.
> 
> OK, I rephrase it,
> "Unparenthesized unary expression found on the left hand side of an exponentiation expression"

I'm not sure if this is quite right, because the user could either mean
(-x)**y
or
-(x**y)

Maybe we should say something like "ambiguous unary expression in the LHS of the exponentiation expression; parenthesis must be used to disambiguate the expression"
I'm not sure exactly what the best message is.
It'd be really cool if we could take the user's program and show them the two options somehow. Probably difficult to do though.

[Yusuke Suzuki]

Comment on attachment 284122 [details]
Patch

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

>>>> Source/JavaScriptCore/parser/Parser.cpp:3274
>>>> +        failIfTrue(match(POW) && isUnaryOpExcludingUpdateOp(leadingTokenTypeForUnaryExpression), "Expected a update expression on the left hand side of an exponentiation expression");
>>> 
>>> I don't think this error message is helpful.
>>> Maybe we can say something about precedence and that the user should explicitly place parens to emphasize precedence.
>> 
>> OK, I rephrase it,
>> "Unparenthesized unary expression found on the left hand side of an exponentiation expression"
> 
> I'm not sure if this is quite right, because the user could either mean
> (-x)**y
> or
> -(x**y)
> 
> Maybe we should say something like "ambiguous unary expression in the LHS of the exponentiation expression; parenthesis must be used to disambiguate the expression"
> I'm not sure exactly what the best message is.
> It'd be really cool if we could take the user's program and show them the two options somehow. Probably difficult to do though.

Sounds nice. I'll change this to "Amiguous unary expression in the left hand side of the exponentiation expression; parenthesis must be used to disambiguate the expression".

[Yusuke Suzuki]

Committed r203664: <http://trac.webkit.org/changeset/203664>