Bug 159969

Summary: [ES7] Introduce exponentiation expression
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch saam: review+

Description Yusuke Suzuki 2016-07-20 04:32:14 PDT 
[ES7] Introduce exponentiation expression (**)
Comment 1 Yusuke Suzuki 2016-07-20 09:07:04 PDT 
Created attachment 284109 [details]
Patch

WIP
Comment 2 Yusuke Suzuki 2016-07-20 09:57:00 PDT 
Created attachment 284115 [details]
Patch
Comment 3 Yusuke Suzuki 2016-07-20 09:58:36 PDT 
Created attachment 284116 [details]
Patch
Comment 4 Yusuke Suzuki 2016-07-20 10:13:32 PDT 
Created attachment 284119 [details]
Patch

Drop unnecessary UNLIKELY and update the performance in ChangeLog
Comment 5 Yusuke Suzuki 2016-07-20 10:26:16 PDT 
Created attachment 284122 [details]
Patch

Fix ChangeLog
Comment 6 Saam Barati 2016-07-20 12:30:05 PDT 
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
Comment 7 Yusuke Suzuki 2016-07-21 00:13:57 PDT 
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.
Comment 8 Yusuke Suzuki 2016-07-21 00:34:31 PDT 
Committed r203499: <http://trac.webkit.org/changeset/203499>
Comment 9 Saam Barati 2016-07-21 12:29:56 PDT 
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.
Comment 10 Yusuke Suzuki 2016-07-22 11:11:29 PDT 
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".
Comment 11 Yusuke Suzuki 2016-07-24 08:54:49 PDT 
Committed r203664: <http://trac.webkit.org/changeset/203664>