[Interpreter] Optimize the Register Optimizer.

[Interpreter] Optimize the Register Optimizer.

Modify the Bytecode Register Optimizer to be an independent component
rather than part of the BytecodePipeline. This means the BytecodeArrayBuilder
can explicitly call it with register operands when outputting a bytecode
and the Bytecode Register Optimizer doesn't need to work out which operands
are register operands. This also means we don't need to build BytecodeNodes
for Ldar / Star / Mov bytecodes unless they are actually emitted by the
optimizer.

This change also modifies the way the BytecodeArrayBuilder converts
operands to make use of the OperandTypes specified in bytecodes.h.
This avoids having to individually convert operands to their raw output
value before calling Output(...).

BUG=v8:4280
Committed: https://crrev.com/ed7bef5b91a70c8bed86e066726ca1cdfc777a96
Cr-Commit-Position: refs/heads/master@{#40543}

[rmcilroy, 2016-10-05 16:04:39]

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

[commit-bot: I haz the power, 2016-10-05 16:04:46]

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

[rmcilroy, 2016-10-05 16:10:17]

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

[commit-bot: I haz the power, 2016-10-05 16:10:27]

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

[rmcilroy, 2016-10-05 16:14:10]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2016-10-05 16:14:16]

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

[commit-bot: I haz the power, 2016-10-05 16:14:26]

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

[rmcilroy, 2016-10-05 16:14:52]

rmcilroy@chromium.org changed reviewers:
+ leszeks@chromium.org, mythria@chromium.org

[rmcilroy, 2016-10-05 16:14:52]

This gives another bump in compile time. PTAL, thanks.

[commit-bot: I haz the power, 2016-10-05 16:19:57]

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

[commit-bot: I haz the power, 2016-10-05 16:19:58]

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

[Leszek Swirski, 2016-10-06 10:35:17]

Optimizing the optimizer? We need to go deeper.

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

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:102:
!Bytecodes::IsWithoutExternalSideEffects(bytecode)) {
not sure what effect this would have on execution time/code size/icache, but
this method could be templated/x-macro-ed on the bytecode too, and specialised
to skip this check for bytecodes with side effects.

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:104:
latest_source_info_.set_invalid();
maybe not for this CL, but this could be done a bit neater by adding a move
constructor to BytecodeSourceInfo which invalidates the move source.

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:120: static uint32_t
Convert(BytecodeArrayBuilder* builder, int value) {
why not INLINE?

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:125: static bool IsValid(size_t value)
{
maybe private?

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:222: bytecode,
OperandHelper<operand_types>::Convert(builder, operands)...,
It might be helpful for future readers of this code, here and below, to write a
short comment demonstrating how the ellipsis unpacks the arguments, because it's
not immediately obvious.

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

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:327: // Used internally to prepare for
emitting a given bytecode.
If this is used internally, should it be private?

[rmcilroy, 2016-10-06 17:14:38]

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

[rmcilroy, 2016-10-06 17:14:43]

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

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:102:
!Bytecodes::IsWithoutExternalSideEffects(bytecode)) {
On 2016/10/06 10:35:17, Leszek Swirski wrote:
> not sure what effect this would have on execution time/code size/icache, but
> this method could be templated/x-macro-ed on the bytecode too, and specialised
> to skip this check for bytecodes with side effects.

Yeah, I made IsWithoutExternalSideEffects a constexpr in a previous CL which
appears to allow the compiler to do this during compile time when it knows the
bytecode which is being passed (i.e., if this is inlined) -this get's the code
improvement without the template code complexity.

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:104:
latest_source_info_.set_invalid();
On 2016/10/06 10:35:17, Leszek Swirski wrote:
> maybe not for this CL, but this could be done a bit neater by adding a move
> constructor to BytecodeSourceInfo which invalidates the move source.

This caused a bunch of issues elsewhere with the deleted copy constructor. I
could also create a copy constructor, but think it would be confusing that the
move constructor invalidates while the copy constructor doesn't, so kept
set_invalid as a seperate call. 

Did remove Clone and replace with plain assignment though.

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:120: static uint32_t
Convert(BytecodeArrayBuilder* builder, int value) {
On 2016/10/06 10:35:17, Leszek Swirski wrote:
> why not INLINE?

Done.

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:125: static bool IsValid(size_t value)
{
On 2016/10/06 10:35:17, Leszek Swirski wrote:
> maybe private?

Done.

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:222: bytecode,
OperandHelper<operand_types>::Convert(builder, operands)...,
On 2016/10/06 10:35:17, Leszek Swirski wrote:
> It might be helpful for future readers of this code, here and below, to write
a
> short comment demonstrating how the ellipsis unpacks the arguments, because
it's
> not immediately obvious.

Done.

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

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:327: // Used internally to prepare for
emitting a given bytecode.
On 2016/10/06 10:35:17, Leszek Swirski wrote:
> If this is used internally, should it be private?

Done.

[commit-bot: I haz the power, 2016-10-06 17:14:45]

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, 2016-10-06 17:40:11]

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

[commit-bot: I haz the power, 2016-10-06 17:40:12]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-10-07 08:34:10]

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

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:51: pipeline_);
I don't love that both this class and the register optimizer now interleave
writes to the pipeline -- but, that said, I can't think of an efficient way to
implement the DoLdr etc. methods without it (I considered suggesting making it a
boolean e.g. NeedsLdr, but that didn't seem any better).

Maybe we should pass the pipeline through in the DoLdr calls, rather than in the
constructor, so that at least the output pipeline is not part of the optimizer's
state and it's explicit that we'll be outputting with it?

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:102:
!Bytecodes::IsWithoutExternalSideEffects(bytecode)) {
On 2016/10/06 17:14:43, rmcilroy wrote:
> On 2016/10/06 10:35:17, Leszek Swirski wrote:
> > not sure what effect this would have on execution time/code size/icache, but
> > this method could be templated/x-macro-ed on the bytecode too, and
specialised
> > to skip this check for bytecodes with side effects.
> 
> Yeah, I made IsWithoutExternalSideEffects a constexpr in a previous CL which
> appears to allow the compiler to do this during compile time when it knows the
> bytecode which is being passed (i.e., if this is inlined) -this get's the code
> improvement without the template code complexity. 

Oh yeah, inlining, stupid me. Did you check if it is actually inlined?

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:104:
latest_source_info_.set_invalid();
On 2016/10/06 17:14:43, rmcilroy wrote:
> On 2016/10/06 10:35:17, Leszek Swirski wrote:
> > maybe not for this CL, but this could be done a bit neater by adding a move
> > constructor to BytecodeSourceInfo which invalidates the move source.
> 
> This caused a bunch of issues elsewhere with the deleted copy constructor. I
> could also create a copy constructor, but think it would be confusing that the
> move constructor invalidates while the copy constructor doesn't, so kept
> set_invalid as a seperate call. 
> 
> Did remove Clone and replace with plain assignment though. 

Fair enough, I might put that refactor on my backlog for some point.

[mythria, 2016-10-07 14:42:14]

Great. It took me a bit of time to understand OperandHelper :) lgtm.

[rmcilroy, 2016-10-24 16:27:55]

PTAL Leszek.

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

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:51: pipeline_);
On 2016/10/07 08:34:10, Leszek Swirski wrote:
> I don't love that both this class and the register optimizer now interleave
> writes to the pipeline -- but, that said, I can't think of an efficient way to
> implement the DoLdr etc. methods without it (I considered suggesting making it
a
> boolean e.g. NeedsLdr, but that didn't seem any better).
> 
> Maybe we should pass the pipeline through in the DoLdr calls, rather than in
the
> constructor, so that at least the output pipeline is not part of the
optimizer's
> state and it's explicit that we'll be outputting with it?

As discussed offline, this isn't super easy. Let's land this as is and update to
add the extra level of indirection in a seperate CL to see if it impacts perf.

https://codereview.chromium.org/2393683004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:102:
!Bytecodes::IsWithoutExternalSideEffects(bytecode)) {
On 2016/10/07 08:34:10, Leszek Swirski wrote:
> On 2016/10/06 17:14:43, rmcilroy wrote:
> > On 2016/10/06 10:35:17, Leszek Swirski wrote:
> > > not sure what effect this would have on execution time/code size/icache,
but
> > > this method could be templated/x-macro-ed on the bytecode too, and
> specialised
> > > to skip this check for bytecodes with side effects.
> > 
> > Yeah, I made IsWithoutExternalSideEffects a constexpr in a previous CL which
> > appears to allow the compiler to do this during compile time when it knows
the
> > bytecode which is being passed (i.e., if this is inlined) -this get's the
code
> > improvement without the template code complexity. 
> 
> Oh yeah, inlining, stupid me. Did you check if it is actually inlined?

Yeah it is, at least for the functions which appear high-up on the perf profile.

[rmcilroy, 2016-10-24 16:28:13]

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

[commit-bot: I haz the power, 2016-10-24 16:28:15]

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

[Leszek Swirski, 2016-10-24 16:29:30]

lgtm

[commit-bot: I haz the power, 2016-10-24 16:56:53]

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

[commit-bot: I haz the power, 2016-10-24 16:56:54]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-10-24 20:42:14]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-10-24 20:42:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mythria@chromium.org
Link to the patchset: https://codereview.chromium.org/2393683004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-24 20:42:24]

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-10-24 20:47:45]

Committed patchset #5 (id:100001)

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

Description was changed from

==========
[Interpreter] Optimize the Register Optimizer.

Modify the Bytecode Register Optimizer to be an independent component
rather than part of the BytecodePipeline. This means the BytecodeArrayBuilder
can explicitly call it with register operands when outputting a bytecode
and the Bytecode Register Optimizer doesn't need to work out which operands
are register operands. This also means we don't need to build BytecodeNodes
for Ldar / Star / Mov bytecodes unless they are actually emitted by the
optimizer.

This change also modifies the way the BytecodeArrayBuilder converts
operands to make use of the OperandTypes specified in bytecodes.h.
This avoids having to individually convert operands to their raw output
value before calling Output(...).

BUG=v8:4280
==========

to

==========
[Interpreter] Optimize the Register Optimizer.

Modify the Bytecode Register Optimizer to be an independent component
rather than part of the BytecodePipeline. This means the BytecodeArrayBuilder
can explicitly call it with register operands when outputting a bytecode
and the Bytecode Register Optimizer doesn't need to work out which operands
are register operands. This also means we don't need to build BytecodeNodes
for Ldar / Star / Mov bytecodes unless they are actually emitted by the
optimizer.

This change also modifies the way the BytecodeArrayBuilder converts
operands to make use of the OperandTypes specified in bytecodes.h.
This avoids having to individually convert operands to their raw output
value before calling Output(...).

BUG=v8:4280

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

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

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