[Interpreter] Add support for calling runtime functions which return a pair.

[Interpreter] Add support for calling runtime functions which return a pair.

Adds support for calling runtime functions which return a pair of
values. Adds the bytecode CallRuntimePair. Also adds support to TurboFan
for calling stubs which return multiple values.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/1a063d944dadb17ade60c2de5a00b15e8f8245cb
Cr-Commit-Position: refs/heads/master@{#33181}

[rmcilroy, 2016-01-06 12:45:43]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2016-01-06 12:47:25]

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

[rmcilroy, 2016-01-06 12:47:26]

Benedikt, please take a look at changes in compiler/
Orion, please take a look at overall change.

The tests for CallRuntimePair are added in
https://codereview.chromium.org/1508293003/ where I use it to implement Eval
support.

[rmcilroy, 2016-01-06 12:50:56]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-06 12:51:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568493002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568493002/20001

[commit-bot: I haz the power, 2016-01-06 13:34:59]

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

[commit-bot: I haz the power, 2016-01-06 13:35:00]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/1269)
  v8_win64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[oth, 2016-01-06 15:09:17]

Looks good modulo query about connection to bytecode generator. 

The name CallRuntimePair is a bit cryptic. CallRuntimeForPair?

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1005: BytecodeArrayBuilder&
BytecodeArrayBuilder::CallRuntimePair(
This doesn't look like it's called except in tests. Is more required to hook it
up to the bytecode generator?

[rmcilroy, 2016-01-06 15:23:25]

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1005: BytecodeArrayBuilder&
BytecodeArrayBuilder::CallRuntimePair(
On 2016/01/06 15:09:17, oth wrote:
> This doesn't look like it's called except in tests. Is more required to hook
it
> up to the bytecode generator?

Yup, as mentioned in the CL message, this is tested in the Eval CL:
https://codereview.chromium.org/1508293003/

[oth, 2016-01-06 16:26:07]

The approach looks fine, but the bytecode is going to cause issues elsewhere as
proposed.

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:145: OperandType::kCount8, OperandType::kReg8)      
                           \
The problem with this bytecode is that the set of operands ends with an implicit
range.

For register-mapping there needs to be a consistent way to identify ranges of
registers, preferably without hardcoding the information for every bytecode. The
current wide register CL does this by looking for a count operand after any
8-bit register operand.

This bytecode could avoid the problem by returning one element in the
accumulator and one element as a register. Or changing the first maybe register
to be a register that takes the result. Neither is pretty, but perhaps better
than increasing the maximum number of operands or having to manually hardcode
patterns of registers.

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

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1065: Node* result_0 = __ Projection(0,
result_pair);
More widely used convention result0, result1, e.g. reg0, reg1, operand0,
operand1.

[rmcilroy, 2016-01-06 16:48:34]

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:145: OperandType::kCount8, OperandType::kReg8)      
                           \
On 2016/01/06 16:26:07, oth wrote:
> The problem with this bytecode is that the set of operands ends with an
implicit
> range.
> 
> For register-mapping there needs to be a consistent way to identify ranges of
> registers, preferably without hardcoding the information for every bytecode.
The
> current wide register CL does this by looking for a count operand after any
> 8-bit register operand.
> 
> This bytecode could avoid the problem by returning one element in the
> accumulator and one element as a register. Or changing the first maybe
register
> to be a register that takes the result. Neither is pretty, but perhaps better
> than increasing the maximum number of operands or having to manually hardcode
> patterns of registers.

We should talk about this when your back in the office tomorrow, however we
can't do one in the accumulator, one in a register for deoptimization reasons -
the values need to be co-located on the stack. 

>The current wide register CL does this by looking for a count operand after any
8-bit register operand.

I think looking for a count after any kReg8 bytecode is pretty brittle. Could we
not have a switch statement on the bytecode which special cases calls (with the
count and first_arg) and this bytecode (with the first_return having a fixed
"count" of 2)? That would seem safer and would mean this bytecode isn't a
problem. WDYT?

[oth, 2016-01-06 18:03:53]

On 2016/01/06 16:48:34, rmcilroy wrote:
>
https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecodes.h:145: OperandType::kCount8, OperandType::kReg8)    
 
>                            \
> On 2016/01/06 16:26:07, oth wrote:
> > The problem with this bytecode is that the set of operands ends with an
> implicit
> > range.
> > 
> > For register-mapping there needs to be a consistent way to identify ranges
of
> > registers, preferably without hardcoding the information for every bytecode.
> The
> > current wide register CL does this by looking for a count operand after any
> > 8-bit register operand.
> > 
> > This bytecode could avoid the problem by returning one element in the
> > accumulator and one element as a register. Or changing the first maybe
> register
> > to be a register that takes the result. Neither is pretty, but perhaps
better
> > than increasing the maximum number of operands or having to manually
hardcode
> > patterns of registers.
> 
> We should talk about this when your back in the office tomorrow, however we
> can't do one in the accumulator, one in a register for deoptimization reasons
-
> the values need to be co-located on the stack. 

Looking at the currently proposed usage, the input and output register ranges
overlap. If we're prepared to double purpose a register argument, there is no
issue modulo de-opt constraints.

> >The current wide register CL does this by looking for a count operand after
any
> 8-bit register operand.
> 
> I think looking for a count after any kReg8 bytecode is pretty brittle. Could
we
> not have a switch statement on the bytecode which special cases calls (with
the
> count and first_arg) and this bytecode (with the first_return having a fixed
> "count" of 2)? That would seem safer and would mean this bytecode isn't a
> problem. WDYT?

The wide register CL will hit unreachable if the rules it expects are violated.
Clearly not perfect.

The proposal is also fragile and a source of super hard to find bugs. Either:

1) the proposed switch statement uses default so new bytecodes could be added
that don't have entries in the table.
2) the table has all the bytecodes in, but if any ever change without the
corresponding switch statement change.

Talk tomorrow.

[Benedikt Meurer, 2016-01-07 04:54:11]

Description was changed from

==========
[Interpreter] Add support for calling runtime functions which return a pair.

Adds support for calling runtime functions which return a pair of
values. Adds the bytecode CallRuntimePair. Also adds support to TurboFan
for calling stubs which return multiple values.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for calling runtime functions which return a pair.

Adds support for calling runtime functions which return a pair of
values. Adds the bytecode CallRuntimePair. Also adds support to TurboFan
for calling stubs which return multiple values.

BUG=v8:4280
LOG=N
==========

[Benedikt Meurer, 2016-01-07 04:54:11]

bmeurer@chromium.org changed reviewers:
+ mstarzinger@chromium.org
- bmeurer@chromium.org

[Benedikt Meurer, 2016-01-07 04:54:11]

bmeurer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2016-01-07 17:21:28]

Approach is looking good. Just one suggestion in two places.

https://codereview.chromium.org/1568493002/diff/20001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/1568493002/diff/20001/src/code-factory.h#newc...
src/code-factory.h:114: static Callable InterpreterCEntryPair(Isolate* isolate);
suggestion: What would you think about passing in the "result_size" as an
argument to InterpreterCEntry (with a default argument value of 1) instead? The
advantage I see is that callers could be parameterized, which would reduce code
duplication slightly.

https://codereview.chromium.org/1568493002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1568493002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:138: Node* CallRuntime(Node* function_id,
Node* first_arg, Node* arg_count);
suggestion: Likewise to code-factory, can we pass "result_size" as an additional
argument? Again, less code duplication.

[rmcilroy, 2016-01-07 17:21:43]

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:145: OperandType::kCount8, OperandType::kReg8)      
                           \
On 2016/01/06 16:48:34, rmcilroy wrote:
> On 2016/01/06 16:26:07, oth wrote:
> > The problem with this bytecode is that the set of operands ends with an
> implicit
> > range.
> > 
> > For register-mapping there needs to be a consistent way to identify ranges
of
> > registers, preferably without hardcoding the information for every bytecode.
> The
> > current wide register CL does this by looking for a count operand after any
> > 8-bit register operand.
> > 
> > This bytecode could avoid the problem by returning one element in the
> > accumulator and one element as a register. Or changing the first maybe
> register
> > to be a register that takes the result. Neither is pretty, but perhaps
better
> > than increasing the maximum number of operands or having to manually
hardcode
> > patterns of registers.
> 
> We should talk about this when your back in the office tomorrow, however we
> can't do one in the accumulator, one in a register for deoptimization reasons
-
> the values need to be co-located on the stack. 
> 
> >The current wide register CL does this by looking for a count operand after
any
> 8-bit register operand.
> 
> I think looking for a count after any kReg8 bytecode is pretty brittle. Could
we
> not have a switch statement on the bytecode which special cases calls (with
the
> count and first_arg) and this bytecode (with the first_return having a fixed
> "count" of 2)? That would seem safer and would mean this bytecode isn't a
> problem. WDYT?

As discussed offline, I've changed this to use a new bytecode operand called
kRegPair8, which should make it clear that this specifies a pair of registers.

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

https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1065: Node* result_0 = __ Projection(0,
result_pair);
On 2016/01/06 16:26:07, oth wrote:
> More widely used convention result0, result1, e.g. reg0, reg1, operand0,
> operand1.

Done.

[oth, 2016-01-08 09:26:12]

On 2016/01/07 17:21:43, rmcilroy wrote:
>
https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecodes.h:145: OperandType::kCount8, OperandType::kReg8)    
 
>                            \
> On 2016/01/06 16:48:34, rmcilroy wrote:
> > On 2016/01/06 16:26:07, oth wrote:
> > > The problem with this bytecode is that the set of operands ends with an
> > implicit
> > > range.
> > > 
> > > For register-mapping there needs to be a consistent way to identify ranges
> of
> > > registers, preferably without hardcoding the information for every
bytecode.
> > The
> > > current wide register CL does this by looking for a count operand after
any
> > > 8-bit register operand.
> > > 
> > > This bytecode could avoid the problem by returning one element in the
> > > accumulator and one element as a register. Or changing the first maybe
> > register
> > > to be a register that takes the result. Neither is pretty, but perhaps
> better
> > > than increasing the maximum number of operands or having to manually
> hardcode
> > > patterns of registers.
> > 
> > We should talk about this when your back in the office tomorrow, however we
> > can't do one in the accumulator, one in a register for deoptimization
reasons
> -
> > the values need to be co-located on the stack. 
> > 
> > >The current wide register CL does this by looking for a count operand after
> any
> > 8-bit register operand.
> > 
> > I think looking for a count after any kReg8 bytecode is pretty brittle.
Could
> we
> > not have a switch statement on the bytecode which special cases calls (with
> the
> > count and first_arg) and this bytecode (with the first_return having a fixed
> > "count" of 2)? That would seem safer and would mean this bytecode isn't a
> > problem. WDYT?
> 
> As discussed offline, I've changed this to use a new bytecode operand called
> kRegPair8, which should make it clear that this specifies a pair of registers.
> 
>
https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/interpr...
> File src/interpreter/interpreter.cc (right):
> 
>
https://codereview.chromium.org/1568493002/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter.cc:1065: Node* result_0 = __ Projection(0,
> result_pair);
> On 2016/01/06 16:26:07, oth wrote:
> > More widely used convention result0, result1, e.g. reg0, reg1, operand0,
> > operand1.
> 
> Done.

Nice, lgtm.

[rmcilroy, 2016-01-08 14:15:07]

https://codereview.chromium.org/1568493002/diff/20001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/1568493002/diff/20001/src/code-factory.h#newc...
src/code-factory.h:114: static Callable InterpreterCEntryPair(Isolate* isolate);
On 2016/01/07 17:21:28, Michael Starzinger wrote:
> suggestion: What would you think about passing in the "result_size" as an
> argument to InterpreterCEntry (with a default argument value of 1) instead?
The
> advantage I see is that callers could be parameterized, which would reduce
code
> duplication slightly.

SGTM, done.

https://codereview.chromium.org/1568493002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1568493002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:138: Node* CallRuntime(Node* function_id,
Node* first_arg, Node* arg_count);
On 2016/01/07 17:21:28, Michael Starzinger wrote:
> suggestion: Likewise to code-factory, can we pass "result_size" as an
additional
> argument? Again, less code duplication.

Done.

[rmcilroy, 2016-01-08 14:33:12]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-08 14:33:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568493002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568493002/80001

[Michael Starzinger, 2016-01-08 14:49:35]

LGTM.

[commit-bot: I haz the power, 2016-01-08 14:58:39]

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

[commit-bot: I haz the power, 2016-01-08 14:58:40]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-01-08 15:02:13]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-01-08 15:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org
Link to the patchset: https://codereview.chromium.org/1568493002/#ps80001
(title: "Address Michi's review comments.")

[commit-bot: I haz the power, 2016-01-08 15:02:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568493002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568493002/80001

[commit-bot: I haz the power, 2016-01-08 15:15:57]

Description was changed from

==========
[Interpreter] Add support for calling runtime functions which return a pair.

Adds support for calling runtime functions which return a pair of
values. Adds the bytecode CallRuntimePair. Also adds support to TurboFan
for calling stubs which return multiple values.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for calling runtime functions which return a pair.

Adds support for calling runtime functions which return a pair of
values. Adds the bytecode CallRuntimePair. Also adds support to TurboFan
for calling stubs which return multiple values.

BUG=v8:4280
LOG=N
==========

[commit-bot: I haz the power, 2016-01-08 15:15:58]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-01-08 15:16:32]

Description was changed from

==========
[Interpreter] Add support for calling runtime functions which return a pair.

Adds support for calling runtime functions which return a pair of
values. Adds the bytecode CallRuntimePair. Also adds support to TurboFan
for calling stubs which return multiple values.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for calling runtime functions which return a pair.

Adds support for calling runtime functions which return a pair of
values. Adds the bytecode CallRuntimePair. Also adds support to TurboFan
for calling stubs which return multiple values.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/1a063d944dadb17ade60c2de5a00b15e8f8245cb
Cr-Commit-Position: refs/heads/master@{#33181}
==========

[commit-bot: I haz the power, 2016-01-08 15:16:32]

Patchset 4 (id:??) landed as
https://crrev.com/1a063d944dadb17ade60c2de5a00b15e8f8245cb
Cr-Commit-Position: refs/heads/master@{#33181}