[ignition,modules] Introduce bytecodes for loading/storing module variables.

[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder exists only in the form of runtime calls.

BUG=v8:1569
Committed: https://crrev.com/dd155e47bd167b4bb536493579366f8035896233
Cr-Commit-Position: refs/heads/master@{#40825}

[adamk, 2016-11-02 18:43:45]

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

[adamk, 2016-11-02 18:43:46]

This looks reasonable to me too for now. I think in future it's quite possible
we'll want to store imports and exports directly in the module context, and once
we do that, we could do the computation of index at compile time.

[neis, 2016-11-03 13:25:08]

Description was changed from

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

BUG=v8:1569
==========

to

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder is still missing.

BUG=v8:1569
==========

[neis, 2016-11-03 13:25:08]

neis@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[neis, 2016-11-03 13:27:51]

Ready for review, PTAL.

[rmcilroy, 2016-11-03 13:58:07]

Looks good. A couple of comments but LGTM once addressed.

https://codereview.chromium.org/2471033004/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2471033004/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:695: void
BytecodeGraphBuilder::VisitStaModuleVariable() { UNIMPLEMENTED(); }
Should these just call the runtime function for now rather than crashing (e.g.,
won't the tests fail in --always-opt)?

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:102: // Load from/store to the module
variable with the given index.
nit - load from accumulator / store accumulator to the...

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:962: __ IntPtrMul(raw_index, __
IntPtrConstant(-1)), __ IntPtrConstant(1));
nit - would IntPtrSub(0, raw_index) be more efficient, or does the instruction
scheduler just do this anyway?

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:999: __ DebugBreak();
Could we do a real abort here with a bailoutid describing the crash.

[neis, 2016-11-03 15:02:25]

https://codereview.chromium.org/2471033004/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2471033004/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:695: void
BytecodeGraphBuilder::VisitStaModuleVariable() { UNIMPLEMENTED(); }
On 2016/11/03 13:58:06, rmcilroy (OOO - slow) wrote:
> Should these just call the runtime function for now rather than crashing
(e.g.,
> won't the tests fail in --always-opt)?

Good idea, just did that (and tested it manually).

Currently our module tests never run with --always-opt. I will look into getting
better coverage next week probably.

[neis, 2016-11-03 15:56:53]

Description was changed from

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder is still missing.

BUG=v8:1569
==========

to

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder is still missing (Turbofan just calls
the runtime functions).

BUG=v8:1569
==========

[neis, 2016-11-03 16:38:44]

On 2016/11/03 15:02:25, neis wrote:
>
https://codereview.chromium.org/2471033004/diff/40001/src/compiler/bytecode-g...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/2471033004/diff/40001/src/compiler/bytecode-g...
> src/compiler/bytecode-graph-builder.cc:695: void
> BytecodeGraphBuilder::VisitStaModuleVariable() { UNIMPLEMENTED(); }
> On 2016/11/03 13:58:06, rmcilroy (OOO - slow) wrote:
> > Should these just call the runtime function for now rather than crashing
> (e.g.,
> > won't the tests fail in --always-opt)?
> 
> Good idea, just did that (and tested it manually).
> 
> Currently our module tests never run with --always-opt. I will look into
getting
> better coverage next week probably.

As a first step in this direction, see
https://codereview.chromium.org/2472143002/

[adamk, 2016-11-03 18:32:33]

lgtm

[neis, 2016-11-04 10:08:43]

Description was changed from

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder is still missing (Turbofan just calls
the runtime functions).

BUG=v8:1569
==========

to

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder exists only in the form of runtime calls.

BUG=v8:1569
==========

[neis, 2016-11-04 10:37:23]

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:102: // Load from/store to the module
variable with the given index.
On 2016/11/03 13:58:06, rmcilroy (OOO - slow) wrote:
> nit - load from accumulator / store accumulator to the...

It loads from the module variable, not from the accumulator.  I tried to make it
clearer now.

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:962: __ IntPtrMul(raw_index, __
IntPtrConstant(-1)), __ IntPtrConstant(1));
On 2016/11/03 13:58:06, rmcilroy (OOO - slow) wrote:
> nit - would IntPtrSub(0, raw_index) be more efficient, or does the instruction
> scheduler just do this anyway?

Good question. Benedikt, can you comment on this?

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:999: __ DebugBreak();
On 2016/11/03 13:58:06, rmcilroy (OOO - slow) wrote:
> Could we do a real abort here with a bailoutid describing the crash.

Done.

[Benedikt Meurer (Google), 2016-11-04 13:12:55]

bmeurer@google.com changed reviewers:
+ bmeurer@google.com

[Benedikt Meurer (Google), 2016-11-04 13:12:56]

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:962: __ IntPtrMul(raw_index, __
IntPtrConstant(-1)), __ IntPtrConstant(1));
On 2016/11/04 10:37:23, neis wrote:
> On 2016/11/03 13:58:06, rmcilroy (OOO - slow) wrote:
> > nit - would IntPtrSub(0, raw_index) be more efficient, or does the
instruction
> > scheduler just do this anyway?
> 
> Good question. Benedikt, can you comment on this?

I agree with Ross.

[neis, 2016-11-04 13:20:15]

On 2016/11/04 13:12:56, Benedikt Meurer (Google) wrote:
>
https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
> File src/interpreter/interpreter.cc (right):
> 
>
https://codereview.chromium.org/2471033004/diff/40001/src/interpreter/interpr...
> src/interpreter/interpreter.cc:962: __ IntPtrMul(raw_index, __
> IntPtrConstant(-1)), __ IntPtrConstant(1));
> On 2016/11/04 10:37:23, neis wrote:
> > On 2016/11/03 13:58:06, rmcilroy (OOO - slow) wrote:
> > > nit - would IntPtrSub(0, raw_index) be more efficient, or does the
> instruction
> > > scheduler just do this anyway?
> > 
> > Good question. Benedikt, can you comment on this?
> 
> I agree with Ross.

You mean 0 - raw_index is more efficient? Ok, then I'll change it.

Can you please also look at the turbofan and code-stub-assembler changes?
THanks.

[Benedikt Meurer, 2016-11-04 16:45:59]

LGTM with nit.

https://codereview.chromium.org/2471033004/diff/80001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2471033004/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:1291: 
Nit: Please add some kind of assert here. It'd be nice to catch bugs via an
assert instead of an endless loop once the native context is reached.

https://codereview.chromium.org/2471033004/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2471033004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1136: // TODO(neis): Don't call the
runtime.
So next step is to introduce JSLoadModuleVariable and JSStoreModuleVariable,
backed by these runtime calls? And then add typed lowering to
LoadElement/StoreElement?

https://codereview.chromium.org/2471033004/diff/80001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2471033004/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:935: void
Interpreter::DoLdaModuleVariable(InterpreterAssembler* assembler) {
Cool! I like this!

[neis, 2016-11-07 13:48:44]

On 2016/11/04 16:45:59, Benedikt Meurer wrote:
>
https://codereview.chromium.org/2471033004/diff/80001/src/code-stub-assembler...
> src/code-stub-assembler.cc:1291: 
> Nit: Please add some kind of assert here. It'd be nice to catch bugs via an
> assert instead of an endless loop once the native context is reached.

Done.
 
>
https://codereview.chromium.org/2471033004/diff/80001/src/compiler/bytecode-g...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/2471033004/diff/80001/src/compiler/bytecode-g...
> src/compiler/bytecode-graph-builder.cc:1136: // TODO(neis): Don't call the
> runtime.
> So next step is to introduce JSLoadModuleVariable and JSStoreModuleVariable,
> backed by these runtime calls? And then add typed lowering to
> LoadElement/StoreElement?

Yes, I think that's one of the next steps.

[neis, 2016-11-07 13:48:44]

On 2016/11/04 16:45:59, Benedikt Meurer wrote:
>
https://codereview.chromium.org/2471033004/diff/80001/src/code-stub-assembler...
> src/code-stub-assembler.cc:1291: 
> Nit: Please add some kind of assert here. It'd be nice to catch bugs via an
> assert instead of an endless loop once the native context is reached.

Done.
 
>
https://codereview.chromium.org/2471033004/diff/80001/src/compiler/bytecode-g...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/2471033004/diff/80001/src/compiler/bytecode-g...
> src/compiler/bytecode-graph-builder.cc:1136: // TODO(neis): Don't call the
> runtime.
> So next step is to introduce JSLoadModuleVariable and JSStoreModuleVariable,
> backed by these runtime calls? And then add typed lowering to
> LoadElement/StoreElement?

Yes, I think that's one of the next steps.

[neis, 2016-11-07 13:52:15]

As discussed offline with bmeurer and mstarzinger today, I changed the new
bytecodes to take the context depth as argument (similar to the context slot
bytecodes), such that Turbofan will be able to use this information. I removed
the LoadModuleContext function again that I had introduced in the
code-stub-assembler.

PTAL.

[rmcilroy, 2016-11-07 16:07:05]

Changes LGTM.

[neis, 2016-11-08 10:34:40]

The CQ bit was checked by neis@chromium.org

[neis, 2016-11-08 10:34:41]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2471033004/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-08 10:34:49]

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, 2016-11-08 11:01:09]

Description was changed from

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder exists only in the form of runtime calls.

BUG=v8:1569
==========

to

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder exists only in the form of runtime calls.

BUG=v8:1569
==========

[commit-bot: I haz the power, 2016-11-08 11:01:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-11-17 22:25:45]

Description was changed from

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder exists only in the form of runtime calls.

BUG=v8:1569
==========

to

==========
[ignition,modules] Introduce bytecodes for loading/storing module variables.

This introduces two new bytecodes LdaModuleVariable and StaModuleVariable,
replacing the corresponding runtime calls.

Support in the bytecode graph builder exists only in the form of runtime calls.

BUG=v8:1569

Committed: https://crrev.com/dd155e47bd167b4bb536493579366f8035896233
Cr-Commit-Position: refs/heads/master@{#40825}
==========

[commit-bot: I haz the power, 2016-11-17 22:25:46]

Patchset 11 (id:??) landed as
https://crrev.com/dd155e47bd167b4bb536493579366f8035896233
Cr-Commit-Position: refs/heads/master@{#40825}