[ignition] desugar async functions/generators/modules in BytecodeGenerator

[ignition] desugar async functions/generators/modules in BytecodeGenerator

Implements the "port-desugaring-to-Ignition" aspects of
https://codereview.chromium.org/2654423004/ without fixing
the actual bug, and leaving much of the AST desugaring (particularly
the desugaring of ReturnStatement and AwaitExpression) alone. This
should create a way to fix the actual bug with a much smaller change.

The `DeclareGeneratorObjectVar` and `DeclarePromiseVar` methods are
added to Scope to make these variables accessible to the
BytecodeGenerator. They will be removed once the Parser no longer
implements the desugaring of Await, Yield*, and function.sent.

This includes the changes of moving the assignment of a named
FunctionExpression's variable to BytecodeGenerator, and additionally
separating the AST desugaring of non-simple parameters, to allow
BytecodeGenerator to control how and when they are visited (necessary
for putting them inside of a generated try/catch/finally block, as
the case may be).

Finally, some helpers are added to the BytecodeGenerator to allow the
generation of Yield code, and try/catch/finally code without AST
desugaring.

BUG=v8:4280
R=neis@chromium.org, rmcilroy@chromium.org, littledan@chromium.org

[caitp, 2017-01-31 02:11:21]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 02:11:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 02:16:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 02:16:19]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[caitp, 2017-01-31 02:38:51]

Description was changed from

==========
[ignition] desugar async functions/generators/modules in BytecodeGenerator

Implements https://codereview.chromium.org/2654423004/ without fixing
the bugs in question. The goal is to implement a platform for that patch
to stand on, so that it's easier to review.

The `DeclareGeneratorObjectVar` and `DeclarePromiseVar` methods are
added to Scope to make these variables accessible to the
BytecodeGenerator. They will be removed once the Parser no longer
implements the desugaring of Await, Yield*, and function.sent.

This includes the changes of moving the assignment of a named
FunctionExpression's variable to BytecodeGenerator, and additionally
separating the AST desugaring of non-simple parameters, to allow
BytecodeGenerator to control how and when they are visited (necessary
for putting them inside of a generated try/catch/finally block, as
the case may be).

Finally, some helpers are added to the BytecodeGenerator to allow the
generation of Yield code, and try/catch/finally code without AST
desugaring.

BUG=v8:4280
R=neis@chromium.org, rmcilroy@chromium.org, littledan@chromium.org
==========

to

==========
[ignition] desugar async functions/generators/modules in BytecodeGenerator

Implements the "port-desugaring-to-Ignition" aspects of
https://codereview.chromium.org/2654423004/ without fixing
the actual bug, and leaving much of the AST desugaring (particularly the
desugaring of ReturnStatement and AwaitExpression) alone. This should create a
way to fix the actual bug with a much smaller change.

The `DeclareGeneratorObjectVar` and `DeclarePromiseVar` methods are
added to Scope to make these variables accessible to the
BytecodeGenerator. They will be removed once the Parser no longer
implements the desugaring of Await, Yield*, and function.sent.

This includes the changes of moving the assignment of a named
FunctionExpression's variable to BytecodeGenerator, and additionally
separating the AST desugaring of non-simple parameters, to allow
BytecodeGenerator to control how and when they are visited (necessary
for putting them inside of a generated try/catch/finally block, as
the case may be).

Finally, some helpers are added to the BytecodeGenerator to allow the
generation of Yield code, and try/catch/finally code without AST
desugaring.

BUG=v8:4280
R=neis@chromium.org, rmcilroy@chromium.org, littledan@chromium.org
==========

[caitp, 2017-01-31 02:39:38]

Description was changed from

==========
[ignition] desugar async functions/generators/modules in BytecodeGenerator

Implements the "port-desugaring-to-Ignition" aspects of
https://codereview.chromium.org/2654423004/ without fixing
the actual bug, and leaving much of the AST desugaring (particularly the
desugaring of ReturnStatement and AwaitExpression) alone. This should create a
way to fix the actual bug with a much smaller change.

The `DeclareGeneratorObjectVar` and `DeclarePromiseVar` methods are
added to Scope to make these variables accessible to the
BytecodeGenerator. They will be removed once the Parser no longer
implements the desugaring of Await, Yield*, and function.sent.

This includes the changes of moving the assignment of a named
FunctionExpression's variable to BytecodeGenerator, and additionally
separating the AST desugaring of non-simple parameters, to allow
BytecodeGenerator to control how and when they are visited (necessary
for putting them inside of a generated try/catch/finally block, as
the case may be).

Finally, some helpers are added to the BytecodeGenerator to allow the
generation of Yield code, and try/catch/finally code without AST
desugaring.

BUG=v8:4280
R=neis@chromium.org, rmcilroy@chromium.org, littledan@chromium.org
==========

to

==========
[ignition] desugar async functions/generators/modules in BytecodeGenerator

Implements the "port-desugaring-to-Ignition" aspects of
https://codereview.chromium.org/2654423004/ without fixing
the actual bug, and leaving much of the AST desugaring (particularly
the desugaring of ReturnStatement and AwaitExpression) alone. This
should create a way to fix the actual bug with a much smaller change.

The `DeclareGeneratorObjectVar` and `DeclarePromiseVar` methods are
added to Scope to make these variables accessible to the
BytecodeGenerator. They will be removed once the Parser no longer
implements the desugaring of Await, Yield*, and function.sent.

This includes the changes of moving the assignment of a named
FunctionExpression's variable to BytecodeGenerator, and additionally
separating the AST desugaring of non-simple parameters, to allow
BytecodeGenerator to control how and when they are visited (necessary
for putting them inside of a generated try/catch/finally block, as
the case may be).

Finally, some helpers are added to the BytecodeGenerator to allow the
generation of Yield code, and try/catch/finally code without AST
desugaring.

BUG=v8:4280
R=neis@chromium.org, rmcilroy@chromium.org, littledan@chromium.org
==========

[caitp, 2017-01-31 02:41:24]

H'okay.

This splits out the "port to Ignition" side of
https://codereview.chromium.org/2654423004/. Hopefully this is easier to review
as tests are passing and the bytecode is very similar (though not identical) to
what was generated previously.

Please take a look, folks.

[caitp, 2017-01-31 19:36:16]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 19:36:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:37:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:37:53]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/16677)

[rmcilroy, 2017-01-31 21:05:26]

On 2017/01/31 02:41:24, caitp wrote:
> H'okay.
> 
> This splits out the "port to Ignition" side of
> https://codereview.chromium.org/2654423004/. Hopefully this is easier to
review
> as tests are passing and the bytecode is very similar (though not identical)
to
> what was generated previously.
> 
> Please take a look, folks.

I've only scanned the code in BytecodeGenerator, but I'm not hugely keen on this
approach overall. It's moving a lot of responsibility for building up implicit
control flow and code structure in the BytecodeGenerator, which breaks the
layering between the AST construction and the bytecode generator IMHO. Have you
thought about doing a more partial desugaring, e.g., producing some additional
higher-level AST nodes (e.g., "ResumablePrologue, ResumableTryBlock,
ResumableFinallyBlock, etc.) which can be inserted into the AST and then
implemented in the BytecodeGenerator?

[caitp, 2017-01-31 21:27:35]

On 2017/01/31 21:05:26, rmcilroy (travelling till 3rd) wrote:
> On 2017/01/31 02:41:24, caitp wrote:
> > H'okay.
> > 
> > This splits out the "port to Ignition" side of
> > https://codereview.chromium.org/2654423004/. Hopefully this is easier to
> review
> > as tests are passing and the bytecode is very similar (though not identical)
> to
> > what was generated previously.
> > 
> > Please take a look, folks.
> 
> I've only scanned the code in BytecodeGenerator, but I'm not hugely keen on
this
> approach overall. It's moving a lot of responsibility for building up implicit
> control flow and code structure in the BytecodeGenerator, which breaks the
> layering between the AST construction and the bytecode generator IMHO. Have
you
> thought about doing a more partial desugaring, e.g., producing some additional
> higher-level AST nodes (e.g., "ResumablePrologue, ResumableTryBlock,
> ResumableFinallyBlock, etc.) which can be inserted into the AST and then
> implemented in the BytecodeGenerator?

I would prefer not to introduce AST nodes which don't introduce constructs in
the language. While it might make sense for `Await` to have its own AST node,
it's better if the AST represents what is parsed.

The implicit try/catch blocks are still determined to be used based on the
contents of the AST, so in that sense, it doesn't really "break the layering",
though it does transfer responsibility to BytecodeGenerator. It's also not clear
how this is any different from other elements which are handled by
BytecodeGenerator, such as the assignment of the new.target variable, the
arguments object, or a rest argument.

Doing this work in BytecodeGenerator in v8 would not be unlike the approach used
in other implementations, and I believe it's a better place to do it (simpler,
does not require introducing new AST nodes which require touching parts of
crankshaft, full-codegen, wasm, etc), is written imperatively (and is thus
simpler to follow/much less of a headache than a complicated AST desugaring),
and potentially improves the actual generated bytecode.

Other than creating new AST nodes which don't represent syntactic constructs, is
there any way to write this that you guys would be happier with? In my view,
it's cleaner, and potentially generates smaller code, and should only get
cleaner once the Parser no longer needs to reference the generator variable or
promise variables.

[rmcilroy, 2017-02-02 00:10:48]

On 2017/01/31 21:27:35, caitp wrote:
> On 2017/01/31 21:05:26, rmcilroy (travelling till 3rd) wrote:
> > On 2017/01/31 02:41:24, caitp wrote:
> > > H'okay.
> > > 
> > > This splits out the "port to Ignition" side of
> > > https://codereview.chromium.org/2654423004/. Hopefully this is easier to
> > review
> > > as tests are passing and the bytecode is very similar (though not
identical)
> > to
> > > what was generated previously.
> > > 
> > > Please take a look, folks.
> > 
> > I've only scanned the code in BytecodeGenerator, but I'm not hugely keen on
> this
> > approach overall. It's moving a lot of responsibility for building up
implicit
> > control flow and code structure in the BytecodeGenerator, which breaks the
> > layering between the AST construction and the bytecode generator IMHO. Have
> you
> > thought about doing a more partial desugaring, e.g., producing some
additional
> > higher-level AST nodes (e.g., "ResumablePrologue, ResumableTryBlock,
> > ResumableFinallyBlock, etc.) which can be inserted into the AST and then
> > implemented in the BytecodeGenerator?
> 
> I would prefer not to introduce AST nodes which don't introduce constructs in
> the language. While it might make sense for `Await` to have its own AST node,
> it's better if the AST represents what is parsed.
> 
> The implicit try/catch blocks are still determined to be used based on the
> contents of the AST, so in that sense, it doesn't really "break the layering",
> though it does transfer responsibility to BytecodeGenerator. It's also not
clear
> how this is any different from other elements which are handled by
> BytecodeGenerator, such as the assignment of the new.target variable, the
> arguments object, or a rest argument.

I don't agree that this is the same as the new.target (or other) variable
assignment - In those cases the AST explicitly tells the bytecode generator that
we have such a variable, and explicitly what variable it should be stored in,
the bytecode generator then just dumbly generates code to do this. The code here
is very different in that it introduces additional implicit try / catch /
finally blocks and adds much more responsibility to the bytecode generator. The
fact that this CL had to change BuildTryCatch to take lamdas describing the try
/ catch blocks instead of a list of AST nodes suggests it significantly changes
the approach taken in the bytecode generator. 

> Doing this work in BytecodeGenerator in v8 would not be unlike the approach
used
> in other implementations, and I believe it's a better place to do it (simpler,
> does not require introducing new AST nodes which require touching parts of
> crankshaft, full-codegen, wasm, etc), is written imperatively (and is thus
> simpler to follow/much less of a headache than a complicated AST desugaring),
> and potentially improves the actual generated bytecode.

I'm not sure I agree that this is simpler, ignoring the changes in the golden
files, this is a net 123 line increase in the code.

> Other than creating new AST nodes which don't represent syntactic constructs,
is
> there any way to write this that you guys would be happier with? In my view,
> it's cleaner, and potentially generates smaller code, and should only get
> cleaner once the Parser no longer needs to reference the generator variable or
> promise variables.

I'm not sure how much this would simplify things (if at all) but one approach I
would be happier with would be having a separate
"resumable_try/catch/finally_block" list of statements on the literal, then the
BytecodeGenerator would walk these statements at the appropriate place.

Do you have a design doc describing moving this desugering into the bytecode
generator? Adam pointed me at
https://docs.google.com/document/d/1MjVjSWxq4RVzdvAIk7V_vZHnlt837fBuu5d2bCBWc...,
but this doesn't mention moving the desugaring.

[caitp, 2017-02-02 00:51:00]

On 2017/02/02 00:10:48, rmcilroy (travelling till 3rd) wrote:
> On 2017/01/31 21:27:35, caitp wrote:
> > On 2017/01/31 21:05:26, rmcilroy (travelling till 3rd) wrote:
> > > On 2017/01/31 02:41:24, caitp wrote:
> > > > H'okay.
> > > > 
> > > > This splits out the "port to Ignition" side of
> > > > https://codereview.chromium.org/2654423004/. Hopefully this is easier to
> > > review
> > > > as tests are passing and the bytecode is very similar (though not
> identical)
> > > to
> > > > what was generated previously.
> > > > 
> > > > Please take a look, folks.
> > > 
> > > I've only scanned the code in BytecodeGenerator, but I'm not hugely keen
on
> > this
> > > approach overall. It's moving a lot of responsibility for building up
> implicit
> > > control flow and code structure in the BytecodeGenerator, which breaks the
> > > layering between the AST construction and the bytecode generator IMHO.
Have
> > you
> > > thought about doing a more partial desugaring, e.g., producing some
> additional
> > > higher-level AST nodes (e.g., "ResumablePrologue, ResumableTryBlock,
> > > ResumableFinallyBlock, etc.) which can be inserted into the AST and then
> > > implemented in the BytecodeGenerator?
> > 
> > I would prefer not to introduce AST nodes which don't introduce constructs
in
> > the language. While it might make sense for `Await` to have its own AST
node,
> > it's better if the AST represents what is parsed.
> > 
> > The implicit try/catch blocks are still determined to be used based on the
> > contents of the AST, so in that sense, it doesn't really "break the
layering",
> > though it does transfer responsibility to BytecodeGenerator. It's also not
> clear
> > how this is any different from other elements which are handled by
> > BytecodeGenerator, such as the assignment of the new.target variable, the
> > arguments object, or a rest argument.
> 
> I don't agree that this is the same as the new.target (or other) variable
> assignment - In those cases the AST explicitly tells the bytecode generator
that
> we have such a variable, and explicitly what variable it should be stored in,
> the bytecode generator then just dumbly generates code to do this. The code
here
> is very different in that it introduces additional implicit try / catch /
> finally blocks and adds much more responsibility to the bytecode generator.
The
> fact that this CL had to change BuildTryCatch to take lamdas describing the
try
> / catch blocks instead of a list of AST nodes suggests it significantly
changes
> the approach taken in the bytecode generator. 
> 
> > Doing this work in BytecodeGenerator in v8 would not be unlike the approach
> used
> > in other implementations, and I believe it's a better place to do it
(simpler,
> > does not require introducing new AST nodes which require touching parts of
> > crankshaft, full-codegen, wasm, etc), is written imperatively (and is thus
> > simpler to follow/much less of a headache than a complicated AST
desugaring),
> > and potentially improves the actual generated bytecode.
> 
> I'm not sure I agree that this is simpler, ignoring the changes in the golden
> files, this is a net 123 line increase in the code.
> 
> > Other than creating new AST nodes which don't represent syntactic
constructs,
> is
> > there any way to write this that you guys would be happier with? In my view,
> > it's cleaner, and potentially generates smaller code, and should only get
> > cleaner once the Parser no longer needs to reference the generator variable
or
> > promise variables.
> 
> I'm not sure how much this would simplify things (if at all) but one approach
I
> would be happier with would be having a separate
> "resumable_try/catch/finally_block" list oif statements on the literal, then
the
> BytecodeGenerator would walk these statements at the appropriate place.

Take the Yield* desugaring for instance, It's tons of AST nodes added, which are
difficult to read in the frontend, and difficult to add things to. And, even if
an AST is sort of  declarative, the AST is not constructed in a declarative way,
so it's very painful to look at or make changes to.

For that reason, short of very small things where it makes some degree of sense
to desugar in the parser, I don't think it makes any sense to add the implicit
control flow for generators and async functions there, and I especially don't
think it makes any sense to duplicate the ControlScope functionality in the AST,
which is essentially what's been proposed.

Instead, I'm trying to write this in a very straightforward way. The lambda
thing in BuildTryCatch/Finally could be replaced with an RAII-style thing to
make it clear that it's a linear thing, I'm okay with that --- but I really
thing instead of adding gunk to the AST, it should just reuse the mechanisms
that already exist for ordinary functions. Why shouldn't they, when they still
work fine?

I also think, because the direct Bytecode desugaring is written imperatively,
it's easier to understand. I admit that it's a bit awkward to have the code for
the various generator/async function./modules mixed and matched like that. In
JSC, they basically have a switch statement and go through each function type
separately, only combining cases which are extremely similar. Something like
that would make the patch bigger, but would probably be less of a pain to
understand.

> 
> Do you have a design doc describing moving this desugering into the bytecode
> generator? Adam pointed me at
>
https://docs.google.com/document/d/1MjVjSWxq4RVzdvAIk7V_vZHnlt837fBuu5d2bCBWc...,
> but this doesn't mention moving the desugaring.

I can go over the design further, but the gist of it is that I don't think this
stuff should be done in the parser, I don't think the parser should spend time
on it, and I don't think it is any easier to work with because it's encoded in
the AST, that really only makes it harder to work with, and more likely to
generate poorer code.

But, I really think it's pretty self-explanatory where I'd like to go with this:
get rid of all the implicit control flow the parser adds, some of which is
duplicated when it can be done in a single top-level try/catch/finally. The
Parser also adds a lot of other stuff to the body of the code (like assignment
of the named-function-expr variable), so that was removed from the parser as
well to simplify doing all of this stuff in a consistent way/order. Instead of
adding a new state machine via the AST, it uses the existing ControlScope stuff
in Ignition/TF, and simply changes how the final return is performed, which I
think is the most straight forward and sensible way to fix that problem, and the
most likely to generate the smallest bytecode.

I can write this up, but it all seems pretty straight forward to me, and it's
not clear why any of this would be unexpected or a problem. I do not want to
have any of this done in the parser, because it makes things more complicated,
especially in 3 weeks or 3 months, so I don't think a `resumable_try_statements`
or `resumable_finally_statements` is any different, in any way, from just adding
the try/catch blocks in the AST.

If I can't sell people on not doing it in the parser, then I think someone else
will need to take over the async iteration project, because I think it's a bad
idea to do it in the parser and won't want to be maintaining it if that's how
it's done.

[caitp, 2017-02-06 19:15:10]

So, from the comment on the proposal document, it sounds like you can live with
the approach in this CL.

That said, would you prefer if it were rewritten so that the 3 different async
function/generator/modules cases were not combined with each other? It would
take more code to do that, but would most likely result in something easier to
understand.

[caitp, 2017-02-06 21:05:45]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 21:05:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-06 21:17:15]

On 2017/02/06 19:15:10, caitp wrote:
> So, from the comment on the proposal document, it sounds like you can live
with
> the approach in this CL.
> 
> That said, would you prefer if it were rewritten so that the 3 different async
> function/generator/modules cases were not combined with each other? It would
> take more code to do that, but would most likely result in something easier to
> understand.

I've taken the liberty of rewriting it that way, I think it's a bit easier to
read and debug, but again, it is at the expense of being more lines of code.
Whatever is preferable

[commit-bot: I haz the power, 2017-02-06 21:34:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 21:34:44]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-02-06 22:27:50]

Some higher level comments.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:745: }
This change doesn't seem related to the generator / resumable change, could it
be made in another CL?

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:755: }
Thanks for splitting the logic out of GenerateBytecodeBody. I'm not keen on
having the VisitStatements operation split out across multiple functions though.
Would something like the following be possible instead:

ResumableFunctionBodyBuilder* resumable_builder;
if (IsResumableFunction(...)) {
  resumable_builder =
ResumableFunctionBodyBuilder::Create(info()->literal()->kind());
  resumable_builder->BuildPrologue();
}
VisitStatements(info()->literal()->body());
if (IsResumableFunction(...)) {
  resumable_builder->BuildEpilogue();
}

With the ResumableFunctionBodyBuilder switching based on the function kind to
decide what prologue / epilogue to generate?

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:761: }
Any reason for this change? Could we just do this unconditionally before any of
the generator alternatives?

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:771: int yield_position =
info()->literal()->position();
Could you pass through literal as an argument rather than getting it from the
info() each time.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:861: if
(builder()->RequiresImplicitReturn()) {
From here to the end shouldn't be necessary (it's done in GenerateBytecode)

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:891: [=]() {
I don't like these lambdas, and the function pointers being passed to
BuildTryCatch. Instead, could you simply augment the TryCatch/FinallyBuilder so
that it does the additional work in VisitTryCatch/Finally (or create new
TryCatch/FinallyBuilders which augment the ones in control-builders if you need
access to the bytecode generator) and then simply use the try/catch builders
directly in the GenerateForAsync/Generator functions.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:942: if
(builder()->RequiresImplicitReturn()) {
Ditto

[caitp, 2017-02-06 22:48:25]

I'll see what I can do about these requests. Answered some questions inline

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:745: }
On 2017/02/06 22:27:50, rmcilroy wrote:
> This change doesn't seem related to the generator / resumable change, could it
> be made in another CL?

well, I would love to do it in a separate CL, but there's a reason it's here:

My reasoning is as follows:

1) In order to do the implicit control-flow magic in BytecodeGenerator,
BytecodeGenerator (and thus the FunctionLiteral AST) need knowledge of stuff
that gets added implicitly to function bodies by the Parser. In particular,
parameter initialization needs to happen within the implicit try-block in async
functions, so that any errors during initialization are propagated to
RejectPromise().

2) It's possible (and there are tests verifying) that the function-named
variable is possible to use during parameter initialization, so needs to be
bound before parameter initialization.

---

So given that, it is unfortunately related. I wouldn't mind submitting it in
separate patches, but that would require changing test262.status and
mjsunit.status temporarily, and it's a relatively small change to do it here. I
think there is a net benefit to this change though, in that it allows omitting
allocation/assignment of the function-var when it's guaranteed to not be used,
which is probably the vast majority of cases.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:755: }
On 2017/02/06 22:27:50, rmcilroy wrote:
> Thanks for splitting the logic out of GenerateBytecodeBody. I'm not keen on
> having the VisitStatements operation split out across multiple functions
though.
> Would something like the following be possible instead:
> 
> ResumableFunctionBodyBuilder* resumable_builder;
> if (IsResumableFunction(...)) {
>   resumable_builder =
> ResumableFunctionBodyBuilder::Create(info()->literal()->kind());
>   resumable_builder->BuildPrologue();
> }
> VisitStatements(info()->literal()->body());
> if (IsResumableFunction(...)) {
>   resumable_builder->BuildEpilogue();
> }
> 
> With the ResumableFunctionBodyBuilder switching based on the function kind to
> decide what prologue / epilogue to generate?

I'm not entirely sure how this would work due to how the implicit
try/catch/finally stuff is done, unless I can make that work with the try/catch
builder change you suggested below. It seems difficult to do that due to the
reliance on RAII ControlScopes.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:761: }
On 2017/02/06 22:27:50, rmcilroy wrote:
> Any reason for this change? Could we just do this unconditionally before any
of
> the generator alternatives?

Async functions need to incorporate the parameter initialization within the
implicit try/catch, as thrown errors need to result in a rejected Promise, so
it's handled separately for each case.

If we can eliminate the need for the try/catch in async functions (IE with an
alternative to Throw/ReThrow that always rejects a Promise), then it should be
possible to share that logic in one place

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:771: int yield_position =
info()->literal()->position();
On 2017/02/06 22:27:50, rmcilroy wrote:
> Could you pass through literal as an argument rather than getting it from the
> info() each time.

For the cases that would use this, aren't they always getting it from info()
anyways? But, I'll do that if that's preferable.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:861: if
(builder()->RequiresImplicitReturn()) {
On 2017/02/06 22:27:50, rmcilroy wrote:
> From here to the end shouldn't be necessary (it's done in GenerateBytecode)

The one in GenerateBytecode uses BuildReturn(), which in the followup patch to
this (which fixes Return for async functions) produces a _lot_ of extra bytecode
which is supposed to be unreachable anyways), so I wanted to add a custom
implicit return here that avoided the full Return stuff.

Of course, an alternative fix would be to just use builder()->Return(); rather
than BuildReturn() in GenerateBytecode(), since I believe there are never any
deferred commands to handle at that point. If I do that, I think I can get rid
of this stuff.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:891: [=]() {
On 2017/02/06 22:27:49, rmcilroy wrote:
> I don't like these lambdas, and the function pointers being passed to
> BuildTryCatch. Instead, could you simply augment the TryCatch/FinallyBuilder
so
> that it does the additional work in VisitTryCatch/Finally (or create new
> TryCatch/FinallyBuilders which augment the ones in control-builders if you
need
> access to the bytecode generator) and then simply use the try/catch builders
> directly in the GenerateForAsync/Generator functions.

I'll give that a try.

[caitp, 2017-02-06 23:29:36]

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:771: int yield_position =
info()->literal()->position();
On 2017/02/06 22:48:23, caitp wrote:
> On 2017/02/06 22:27:50, rmcilroy wrote:
> > Could you pass through literal as an argument rather than getting it from
the
> > info() each time.
> 
> For the cases that would use this, aren't they always getting it from info()
> anyways? But, I'll do that if that's preferable.

nvm, thought this was specifically about yield_position, but I guess there's
other uses in this function

[caitp, 2017-02-07 03:49:16]

On 2017/02/06 23:29:36, caitp wrote:
>
https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-generator.cc:771: int yield_position =
> info()->literal()->position();
> On 2017/02/06 22:48:23, caitp wrote:
> > On 2017/02/06 22:27:50, rmcilroy wrote:
> > > Could you pass through literal as an argument rather than getting it from
> the
> > > info() each time.
> > 
> > For the cases that would use this, aren't they always getting it from info()
> > anyways? But, I'll do that if that's preferable.
> 
> nvm, thought this was specifically about yield_position, but I guess there's
> other uses in this function

Okay, I've replaced the lambdas with some new RAII types which contain most of
the logic that used to be in VisitTryCatch/Finally. I'm not sure this is
actually cleaner, but it's one way to go.

The main problem with doing it like this is, logic is not all in a central
location, so it's harder to follow. Otherwise, I suppose it's clean enough to
work with.

Also made some other requested changes.

[caitp, 2017-02-07 03:49:25]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 03:49:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 05:49:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 05:49:54]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)

[rmcilroy, 2017-02-07 17:24:42]

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:745: }
On 2017/02/06 22:48:23, caitp wrote:
> On 2017/02/06 22:27:50, rmcilroy wrote:
> > This change doesn't seem related to the generator / resumable change, could
it
> > be made in another CL?
> 
> well, I would love to do it in a separate CL, but there's a reason it's here:
> 
> My reasoning is as follows:
> 
> 1) In order to do the implicit control-flow magic in BytecodeGenerator,
> BytecodeGenerator (and thus the FunctionLiteral AST) need knowledge of stuff
> that gets added implicitly to function bodies by the Parser. In particular,
> parameter initialization needs to happen within the implicit try-block in
async
> functions, so that any errors during initialization are propagated to
> RejectPromise().
> 
> 2) It's possible (and there are tests verifying) that the function-named
> variable is possible to use during parameter initialization, so needs to be
> bound before parameter initialization.
> 
> ---
> 
> So given that, it is unfortunately related. I wouldn't mind submitting it in
> separate patches, but that would require changing test262.status and
> mjsunit.status temporarily, and it's a relatively small change to do it here.
I
> think there is a net benefit to this change though, in that it allows omitting
> allocation/assignment of the function-var when it's guaranteed to not be used,
> which is probably the vast majority of cases.

OK I can live with this change in this CL, thanks for the explination.

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:755: }
On 2017/02/06 22:48:23, caitp wrote:
> On 2017/02/06 22:27:50, rmcilroy wrote:
> > Thanks for splitting the logic out of GenerateBytecodeBody. I'm not keen on
> > having the VisitStatements operation split out across multiple functions
> though.
> > Would something like the following be possible instead:
> > 
> > ResumableFunctionBodyBuilder* resumable_builder;
> > if (IsResumableFunction(...)) {
> >   resumable_builder =
> > ResumableFunctionBodyBuilder::Create(info()->literal()->kind());
> >   resumable_builder->BuildPrologue();
> > }
> > VisitStatements(info()->literal()->body());
> > if (IsResumableFunction(...)) {
> >   resumable_builder->BuildEpilogue();
> > }
> > 
> > With the ResumableFunctionBodyBuilder switching based on the function kind
to
> > decide what prologue / epilogue to generate?
> 
> I'm not entirely sure how this would work due to how the implicit
> try/catch/finally stuff is done, unless I can make that work with the
try/catch
> builder change you suggested below. It seems difficult to do that due to the
> reliance on RAII ControlScopes.

Yeah your right regarding RAII ControlScopes. I'd still prefer this was an
if/else chain with the default case being in an else at the end (and no
fallthrough)

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:761: }
On 2017/02/06 22:48:24, caitp wrote:
> On 2017/02/06 22:27:50, rmcilroy wrote:
> > Any reason for this change? Could we just do this unconditionally before any
> of
> > the generator alternatives?
> 
> Async functions need to incorporate the parameter initialization within the
> implicit try/catch, as thrown errors need to result in a rejected Promise, so
> it's handled separately for each case.
> 
> If we can eliminate the need for the try/catch in async functions (IE with an
> alternative to Throw/ReThrow that always rejects a Promise), then it should be
> possible to share that logic in one place

I see. Is it necessary that the parameter initialization for generators is
out-with the try/finally? Or could we move it within the Try - either after the
generator creation, or update the finally block to check whether the generator
was created before closing it? If that was possible then we could have a
VisitParameterIntialiationAndStatements() function which did both of these
things and was called from the correct place here and in the resumable variants,
WDYT?

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:771: int yield_position =
info()->literal()->position();
On 2017/02/06 23:29:36, caitp wrote:
> On 2017/02/06 22:48:23, caitp wrote:
> > On 2017/02/06 22:27:50, rmcilroy wrote:
> > > Could you pass through literal as an argument rather than getting it from
> the
> > > info() each time.
> > 
> > For the cases that would use this, aren't they always getting it from info()
> > anyways? But, I'll do that if that's preferable.
> 
> nvm, thought this was specifically about yield_position, but I guess there's
> other uses in this function

Yeah I was meaning for all the uses in the new functions (including
GenerateBytecodeBodyForX

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:861: if
(builder()->RequiresImplicitReturn()) {
On 2017/02/06 22:48:24, caitp wrote:
> On 2017/02/06 22:27:50, rmcilroy wrote:
> > From here to the end shouldn't be necessary (it's done in GenerateBytecode)
> 
> The one in GenerateBytecode uses BuildReturn(), which in the followup patch to
> this (which fixes Return for async functions) produces a _lot_ of extra
bytecode
> which is supposed to be unreachable anyways), so I wanted to add a custom
> implicit return here that avoided the full Return stuff.
> 
> Of course, an alternative fix would be to just use builder()->Return(); rather
> than BuildReturn() in GenerateBytecode(), since I believe there are never any
> deferred commands to handle at that point. If I do that, I think I can get rid
> of this stuff.

Let's make that change in the followup CL which changes what BuildReturn does
please.

https://codereview.chromium.org/2664083002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2664083002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:202: BytecodeGenerator* generator_ =
nullptr;
Why was this change made? The constructor should set generator shouldn't it?

https://codereview.chromium.org/2664083002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:618: }
I'm not sure why we need to update the catch prediction on the generator here -
what changed that means we need to keep track of this on the generator? Can't we
just keep track of the current handler's catch prediction?

https://codereview.chromium.org/2664083002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:643: class
BytecodeGenerator::SimpleTryFinallyBuilder : public TryFinallyBuilder {
I'm not sure on the naming, why is this SimpleTryFinallyBuilder (is there a more
complex version?). Also I'd prefer not to have to extend the TryFinallyBuilder
in control-flow-builders given that they are only used by the code here. Would
it be possible to move the logic into the control-flow-builder variant and only
have a single TryCatch/FinallyBuilder? I think this would be easier if the
comment regarding catch prediction above is addressed since then you should only
need to pass a register to the builder and don't need the bytecode generator.

https://codereview.chromium.org/2664083002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:730: class
BytecodeGenerator::TryBlockBuilder {
I don't think the scoped builders are necessary, I think it would be simpler and
clearer just to explicitly have BeginTry / LeaveTry / BeginHandler /
BeginFinally / EndFinally etc. (and maybe some asserts in each to ensure that
they have all been called when the builder is destroyed).

[caitp, 2017-02-07 18:13:08]

Thanks for the look --- as noted inline, I think I'm going to take a break from
this particular CL for a while (or at least not be able to land it upstream for
a while), due to not wanting to force more code that was previously on the
FCG/CS pipeline onto ignition (particularly named FunctionExpressions)

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:745: }
On 2017/02/07 17:24:41, rmcilroy wrote:
> On 2017/02/06 22:48:23, caitp wrote:
> > On 2017/02/06 22:27:50, rmcilroy wrote:
> > > This change doesn't seem related to the generator / resumable change,
could
> it
> > > be made in another CL?
> > 
> > well, I would love to do it in a separate CL, but there's a reason it's
here:
> > 
> > My reasoning is as follows:
> > 
> > 1) In order to do the implicit control-flow magic in BytecodeGenerator,
> > BytecodeGenerator (and thus the FunctionLiteral AST) need knowledge of stuff
> > that gets added implicitly to function bodies by the Parser. In particular,
> > parameter initialization needs to happen within the implicit try-block in
> async
> > functions, so that any errors during initialization are propagated to
> > RejectPromise().
> > 
> > 2) It's possible (and there are tests verifying) that the function-named
> > variable is possible to use during parameter initialization, so needs to be
> > bound before parameter initialization.
> > 
> > ---
> > 
> > So given that, it is unfortunately related. I wouldn't mind submitting it in
> > separate patches, but that would require changing test262.status and
> > mjsunit.status temporarily, and it's a relatively small change to do it
here.
> I
> > think there is a net benefit to this change though, in that it allows
omitting
> > allocation/assignment of the function-var when it's guaranteed to not be
used,
> > which is probably the vast majority of cases.
> 
> OK I can live with this change in this CL, thanks for the explination.

Actually, I brought this up in the meeting today, and Adam would prefer if I
didn't prevent common code (named function expressions where the function-named
variable is actually used) from using the full-codegen/crankshaft path in this
release.

So, if I want to do this, I either need to implement it in FCG/Crankshaft (which
shouldn't be too hard, but I suspect nobody wants me to do this), or hold back
until --turbo is enabled by default.

For now, I'll try to go with a simpler change. Will open another CL shortly.
I'll keep this one open in case anyone decides it's okay to make this change or
okay to add code handling it in FCG/CS

[rmcilroy, 2017-02-07 20:48:33]

On 2017/02/07 18:13:08, caitp wrote:
> Thanks for the look --- as noted inline, I think I'm going to take a break
from
> this particular CL for a while (or at least not be able to land it upstream
for
> a while), due to not wanting to force more code that was previously on the
> FCG/CS pipeline onto ignition (particularly named FunctionExpressions)
> 
>
https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/2664083002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-generator.cc:745: }
> On 2017/02/07 17:24:41, rmcilroy wrote:
> > On 2017/02/06 22:48:23, caitp wrote:
> > > On 2017/02/06 22:27:50, rmcilroy wrote:
> > > > This change doesn't seem related to the generator / resumable change,
> could
> > it
> > > > be made in another CL?
> > > 
> > > well, I would love to do it in a separate CL, but there's a reason it's
> here:
> > > 
> > > My reasoning is as follows:
> > > 
> > > 1) In order to do the implicit control-flow magic in BytecodeGenerator,
> > > BytecodeGenerator (and thus the FunctionLiteral AST) need knowledge of
stuff
> > > that gets added implicitly to function bodies by the Parser. In
particular,
> > > parameter initialization needs to happen within the implicit try-block in
> > async
> > > functions, so that any errors during initialization are propagated to
> > > RejectPromise().
> > > 
> > > 2) It's possible (and there are tests verifying) that the function-named
> > > variable is possible to use during parameter initialization, so needs to
be
> > > bound before parameter initialization.
> > > 
> > > ---
> > > 
> > > So given that, it is unfortunately related. I wouldn't mind submitting it
in
> > > separate patches, but that would require changing test262.status and
> > > mjsunit.status temporarily, and it's a relatively small change to do it
> here.
> > I
> > > think there is a net benefit to this change though, in that it allows
> omitting
> > > allocation/assignment of the function-var when it's guaranteed to not be
> used,
> > > which is probably the vast majority of cases.
> > 
> > OK I can live with this change in this CL, thanks for the explination.
> 
> Actually, I brought this up in the meeting today, and Adam would prefer if I
> didn't prevent common code (named function expressions where the
function-named
> variable is actually used) from using the full-codegen/crankshaft path in this
> release.
> 
> So, if I want to do this, I either need to implement it in FCG/Crankshaft
(which
> shouldn't be too hard, but I suspect nobody wants me to do this), or hold back
> until --turbo is enabled by default.
> 
> For now, I'll try to go with a simpler change. Will open another CL shortly.
> I'll keep this one open in case anyone decides it's okay to make this change
or
> okay to add code handling it in FCG/CS

OK no problem thanks for letting me know. Let me know if / when you want to
start looking at this CL again.