Implement experimental exponentiation operator

Implement experimental exponentiation operator

BUG=v8:3915
LOG=N
R=

[caitp (gmail), 2015-02-24 03:32:58]

caitpotter88@gmail.com changed reviewers:
+ arv@chromium.org, dslomov@chromium.org, mstarzinger@chromium.org,
waldron.rick@gmail.com

[caitp (gmail), 2015-02-24 03:32:58]

PTAL at your leisure

[Michael Starzinger, 2015-02-24 14:09:04]

Two high-level comments:

1) Why is this being implemented now? Did I miss an "intent to implement" mail?
This is not on track for ES6, correct?

2) The test coverage for TurboFan is very slim (as in non-existent). We should
at least make sure to have unit-tests for the operators and the typer as well as
end-to-end tests in test-run-jsops.

https://codereview.chromium.org/953563002/diff/40001/src/compiler/opcodes.h
File src/compiler/opcodes.h (right):

https://codereview.chromium.org/953563002/diff/40001/src/compiler/opcodes.h#n...
src/compiler/opcodes.h:156: V(NumberExponentiate)       \
This opcode is never used.

https://codereview.chromium.org/953563002/diff/40001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/953563002/diff/40001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:1229: return Type::Range(-V8_INFINITY, +V8_INFINITY,
t->zone());
This is a pretty broad range. :)

https://codereview.chromium.org/953563002/diff/40001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:1234: if (lhs->Is(Type::NaN()) || rhs->Is(Type::NaN()))
return Type::NaN();
Isn't the exponentiation supposed to perform an implicit ToNumber conversion (or
something similar)? This doesn't seem to be reflected here.

[caitp (gmail), 2015-02-24 14:56:13]

There's no intent thread yet, just a prototype. If it bumps up from stage 0 I
will submit an intent thread. In the mean time, it didn't take very long to
write, and I'm using it to get a bit more familiar with the compiler internals,
so it's win/win I think.

About TF coverage, good point.

https://codereview.chromium.org/953563002/diff/40001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/953563002/diff/40001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:1229: return Type::Range(-V8_INFINITY, +V8_INFINITY,
t->zone());
On 2015/02/24 14:09:03, Michael Starzinger wrote:
> This is a pretty broad range. :)

I have some ideas for how to make it a bit more narrow, but it's still going to
be [-ε...-1], [ε...1], [-ε...-Infinity], [ε...Infinity] --- broad range seemed
easiest to get working quickly.

https://codereview.chromium.org/953563002/diff/40001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:1234: if (lhs->Is(Type::NaN()) || rhs->Is(Type::NaN()))
return Type::NaN();
On 2015/02/24 14:09:03, Michael Starzinger wrote:
> Isn't the exponentiation supposed to perform an implicit ToNumber conversion
(or
> something similar)? This doesn't seem to be reflected here.

I might be misunderstanding what the typer is doing. I think I'm determining a
type for the node output, based on the input nodes? Is there a way to tell the
input nodes they need to be converted to numbers here?

[titzer, 2015-02-24 18:46:21]

titzer@chromium.org changed reviewers:
+ titzer@chromium.org

[titzer, 2015-02-24 18:46:22]

https://codereview.chromium.org/953563002/diff/100001/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/953563002/diff/100001/src/compiler/js-operato...
src/compiler/js-operator.h:221: const Operator* Exponentiate();
If this is an experimental feature, it'd be good to keep its footprint in the
compiler as small as possible. Since this is implemented as a runtime call
anyway, I would suggest dropping the operators and type rules and just make the
graph builder put in a runtime call from the beginning.

[Michael Starzinger, 2015-02-24 19:33:31]

On 2015/02/24 14:56:13, caitp wrote:
> There's no intent thread yet, just a prototype. If it bumps up from stage 0 I
> will submit an intent thread. In the mean time, it didn't take very long to
> write, and I'm using it to get a bit more familiar with the compiler
internals,
> so it's win/win I think.

I am not sure how to interpret this. Does this mean that this CL is not intended
to be landed (in let's say the next month)? Am I still supposed to do a thorough
review? Also how will I know when this CL is going from "prototype" to
"implementation"? It's probably just me who is confused at this point.

>
https://codereview.chromium.org/953563002/diff/40001/src/compiler/typer.cc#ne...
> src/compiler/typer.cc:1234: if (lhs->Is(Type::NaN()) || rhs->Is(Type::NaN()))
> return Type::NaN();
> On 2015/02/24 14:09:03, Michael Starzinger wrote:
> > Isn't the exponentiation supposed to perform an implicit ToNumber conversion
> (or
> > something similar)? This doesn't seem to be reflected here.
> 
> I might be misunderstanding what the typer is doing. I think I'm determining a
> type for the node output, based on the input nodes? Is there a way to tell the
> input nodes they need to be converted to numbers here?

Yes, the typer should type a "JSExponentiate" operation here. But since this
operation is of the "JS" flavor and hence performs implicit type conversion, the
typer needs to treat e.g. an operand of "undefined" as "NaN" here. This would be
done by the Typer::Visitor::ToNumber helper.

[caitp (gmail), 2015-02-24 19:36:59]

On 2015/02/24 19:33:31, Michael Starzinger wrote:
> On 2015/02/24 14:56:13, caitp wrote:
> > There's no intent thread yet, just a prototype. If it bumps up from stage 0
I
> > will submit an intent thread. In the mean time, it didn't take very long to
> > write, and I'm using it to get a bit more familiar with the compiler
> internals,
> > so it's win/win I think.
> 
> I am not sure how to interpret this. Does this mean that this CL is not
intended
> to be landed (in let's say the next month)? Am I still supposed to do a
thorough
> review? Also how will I know when this CL is going from "prototype" to
> "implementation"? It's probably just me who is confused at this point.

Actually, I think I was confused --- apparently this proposal has been at stage
2 since last September (based on
https://github.com/tc39/ecma262/commit/1af3d1e7c24343e6c30bd60bd8b7b0c69ce8bd01),
so this is probably due for an intent thread. The proposal itself was marked as
stage 0 until last night
(https://github.com/rwaldron/exponentiation-operator/commit/bffe813e0141a29173...).
So I will end up writing an intent thread this week.

[Dmitry Lomov (no reviews), 2015-02-25 11:21:18]

On 2015/02/24 19:36:59, caitp wrote:
> On 2015/02/24 19:33:31, Michael Starzinger wrote:
> > On 2015/02/24 14:56:13, caitp wrote:
> > > There's no intent thread yet, just a prototype. If it bumps up from stage
0
> I
> > > will submit an intent thread. In the mean time, it didn't take very long
to
> > > write, and I'm using it to get a bit more familiar with the compiler
> > internals,
> > > so it's win/win I think.
> > 
> > I am not sure how to interpret this. Does this mean that this CL is not
> intended
> > to be landed (in let's say the next month)? Am I still supposed to do a
> thorough
> > review? Also how will I know when this CL is going from "prototype" to
> > "implementation"? It's probably just me who is confused at this point.
> 
> Actually, I think I was confused --- apparently this proposal has been at
stage
> 2 since last September (based on
>
https://github.com/tc39/ecma262/commit/1af3d1e7c24343e6c30bd60bd8b7b0c69ce8bd01),
> so this is probably due for an intent thread. The proposal itself was marked
as
> stage 0 until last night
>
(https://github.com/rwaldron/exponentiation-operator/commit/bffe813e0141a29173...).
> So I will end up writing an intent thread this week.

My 2 cents:
1. Writing experimental prototype CLs and seeking feedback is fine and greatly
encouraged as long as those CLs are clearly marked as such. 
2. I am not convinced that now is the right time to land exponentiation operator
in V8:
   - we are in time of great churn, what with all the ES6 features and TurboFan
trying to replace Crankshaft
   - you see for yourself that this Cl ends up being pretty invasive and a
moving part on top of a bunch of other moving parts
   - it is not that we see a high demand for exponentiation operator being
implemented in V8 at this point
   - once this lands, we need to support exp operator throughout any and all
changes and refactorings we need to do. To really get exp operator support on
par with other arithmetic operators we have, we would need to implement
BinaryOpIC support for it and so on and so forth - I do not think we have
bandwidth at this point to do this.

So my recommendation would be to not land this at this point of time.

[rossberg, 2015-02-25 12:34:18]

On 2015/02/25 11:21:18, Dmitry Lomov (chromium) wrote:
> 1. Writing experimental prototype CLs and seeking feedback is fine and greatly
> encouraged as long as those CLs are clearly marked as such. 
> 2. I am not convinced that now is the right time to land exponentiation
operator
> in V8:
>    - we are in time of great churn, what with all the ES6 features and
TurboFan
> trying to replace Crankshaft
>    - you see for yourself that this Cl ends up being pretty invasive and a
> moving part on top of a bunch of other moving parts
>    - it is not that we see a high demand for exponentiation operator being
> implemented in V8 at this point
>    - once this lands, we need to support exp operator throughout any and all
> changes and refactorings we need to do. To really get exp operator support on
> par with other arithmetic operators we have, we would need to implement
> BinaryOpIC support for it and so on and so forth - I do not think we have
> bandwidth at this point to do this.
> 
> So my recommendation would be to not land this at this point of time.

I agree with what Dmitry said. A new primitive operator is a rather intrusive
feature, and we have more than enough loose ends to deal with at the moment. I
prefer to focus on our current priorities (ES6, Turbofan, strong mode), and
avoid broadening the surface of the language & system further before we are
through with all that.

That said, we of course welcome experimentation. To address Michael's concerns,
it would help if the description of experimental CLs clearly stated the
intention (in particular, when there is no intent to land; an explicit heads-up
reply is appreciated once such a CL is emerging from experimental to "real").

[caitp (gmail), 2015-02-25 15:23:31]

On 2015/02/25 12:34:18, rossberg wrote:
> On 2015/02/25 11:21:18, Dmitry Lomov (chromium) wrote:
> > 1. Writing experimental prototype CLs and seeking feedback is fine and
greatly
> > encouraged as long as those CLs are clearly marked as such. 
> > 2. I am not convinced that now is the right time to land exponentiation
> operator
> > in V8:
> >    - we are in time of great churn, what with all the ES6 features and
> TurboFan
> > trying to replace Crankshaft
> >    - you see for yourself that this Cl ends up being pretty invasive and a
> > moving part on top of a bunch of other moving parts
> >    - it is not that we see a high demand for exponentiation operator being
> > implemented in V8 at this point
> >    - once this lands, we need to support exp operator throughout any and all
> > changes and refactorings we need to do. To really get exp operator support
on
> > par with other arithmetic operators we have, we would need to implement
> > BinaryOpIC support for it and so on and so forth - I do not think we have
> > bandwidth at this point to do this.
> > 
> > So my recommendation would be to not land this at this point of time.
> 
> I agree with what Dmitry said. A new primitive operator is a rather intrusive
> feature, and we have more than enough loose ends to deal with at the moment. I
> prefer to focus on our current priorities (ES6, Turbofan, strong mode), and
> avoid broadening the surface of the language & system further before we are
> through with all that.
> 
> That said, we of course welcome experimentation. To address Michael's
concerns,
> it would help if the description of experimental CLs clearly stated the
> intention (in particular, when there is no intent to land; an explicit
heads-up
> reply is appreciated once such a CL is emerging from experimental to "real").

So I'll just do the desugar-to-runtime-call thing in parsing and get rid of all
the compiler changes. it's a pretty trivial, this just makes the parser and AST
a bit weirder than they should be. There's no real requirement that this touches
the compiler

[titzer, 2015-03-10 13:49:56]

titzer@chromium.org changed reviewers:
- titzer@chromium.org