[interpreter] Introduce bytecode generation pipeline.

[interpreter] Introduce bytecode generation pipeline.

This change introduces a pipeline for the final stages of
bytecode generation.

The peephole optimizer is made distinct from the BytecodeArrayBuilder.

A new BytecodeArrayWriter is responsible for writing bytecode. It
also keeps track of the maximum register seen and offers a potentially
smaller frame size.

R=rmcilroy@chromium.org
LOG=N
BUG=v8:4280
Committed: https://crrev.com/02b7373ab135a516c4f553a2f553d086d4c12579
Cr-Commit-Position: refs/heads/master@{#36220}

[oth, 2016-05-05 15:03:06]

Ross, PTAL.

This is already quite large and there are more tests to follow. A couple of
debugger tests to iron out having just rebased against ToT, but will nix those
shortly.

Thanks

[oth, 2016-05-05 15:06:22]

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

[commit-bot: I haz the power, 2016-05-05 15:06:32]

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

[commit-bot: I haz the power, 2016-05-05 15:11:27]

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

[commit-bot: I haz the power, 2016-05-05 15:11:28]

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

[oth, 2016-05-05 21:12:27]

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

[commit-bot: I haz the power, 2016-05-05 21:12:36]

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

[oth, 2016-05-05 21:15:07]

It struck that inserting NOPs simplifies the peephole optimizer and the
interaction with source positions. There are a few tests where unnecessary NOPs
are being inserted, but these are hopefully fixable.

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

https://codereview.chromium.org/1947403002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:256: /* eliminated bytecodess). */                  
                            \
TODO(oth): typo

[commit-bot: I haz the power, 2016-05-05 21:17:05]

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

[commit-bot: I haz the power, 2016-05-05 21:17:06]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/6766)

[oth, 2016-05-06 08:44:05]

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

[commit-bot: I haz the power, 2016-05-06 08:44:16]

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

[commit-bot: I haz the power, 2016-05-06 08:51:49]

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

[commit-bot: I haz the power, 2016-05-06 08:51:50]

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/6893)

[oth, 2016-05-06 09:59:17]

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

[commit-bot: I haz the power, 2016-05-06 09:59:26]

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

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

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

[commit-bot: I haz the power, 2016-05-06 10:29:17]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-05-06 15:25:49]

Overall looks nice. I've got a couple of high-level comments.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:72: BytecodeNode*
BytecodePeepholeOptimizer::Optimize(BytecodeNode* current) {
This function is a bit hard for me to reason about (and will be even more
complicated once we add things like merging bytecodes). I wonder if we could
structure it something like this:

{ 
  UpdateCurrentBytecode(current);

  if (CanElideCurrent(current)) {
    if (current->source_info().is_valid()) {
      // <comment>
      current->set_bytecode(Bytecode::kNop);
    } else {
      current->Release();
      current = nullptr;
    }
    return;
  }
  
  if (last_is_discardable_ && CanElideLast(current)) {
    if (last_->source_info().is_valid()) {
      current->source_info().Update(last_->source_info());
    }
    last_->Release();
    last_ == nullptr;
  }
  return current;
}

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:76: // can only be discarded if
last_is_discardable_ == true.
This comment isn't really true given the source position requirements.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:79: current->set_bytecode(
nit - could we have a replace_bytecode() in BytecodeNode which checks that a
particular substitution is valid, and keeps operands / operand scales the same?

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:109:
!current->source_info().is_statement()) {
It's a bit weird that this is the only is_statement() check. Shouldn't we just
always never elide the last bytecode if both the last bytecode and the current
bytecode have statement positions (otherwise we would swallow  a statement).
This would probably be easier with the CanElideLast() function structure
discussed above.

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

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.cc:18: entry.source_position() >
source_position())) {
Could we get rid of the whole candidate_ source position entry logic in
source-position-table-builder now that this is dealt with here?

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.cc:94: void BytecodeNode::Print(std::ostream&
os) {
Unused - is this useful for debugging?

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-writer.h (right):

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:23: class BytecodeSourceInfo final {
Could we just use PositionTableEntry from source-position-table here instead
please (and add any functions you need into that) and then we can modify
SourcePositionTableBuilder to accept PositionTableEntry's instead of raw
positions values.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:115: class BytecodeNodeAllocator final {
I'd be really happy if we could avoid having to have a BytecodeNodeAllocator. In
this CL AFAIKT you are only really keeping a maximum of two BytecodeNodes around
(last_ in peephole and the current bytecode). Would there be a perf impact to
simply copying the node in peephole optimizer (perhaps you could only keep last_
if there is a potential optimization you could do with that bytecode, otherwise
flush it through immediately)? My gut feeling is that the overhead of the
copying will be offset by the fact that you no longer need to maintain the
freelist. Not sure how this would play into the REO optimizer though.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:138: class BytecodeWriter {
How about calling this BytecodePipelineStage, and renaming
FinalStageBytecodeWriter as BytecodeArrayWriterStage or similar?

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:156: class FinalStageBytecodeWriter : public
BytecodeWriter {
Nit - could we move this to it's own file.

[oth, 2016-05-09 11:02:17]

Thanks for the feedback.

Incorporated nearly everything:

i) Checking out the perf implications of stack allocated BytecodeNodes is TODO.
ii) Not keen on unifying source position types.

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

https://codereview.chromium.org/1947403002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:256: /* eliminated bytecodess). */                  
                            \
On 2016/05/05 21:15:07, oth wrote:
> TODO(oth): typo

Done.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:72: BytecodeNode*
BytecodePeepholeOptimizer::Optimize(BytecodeNode* current) {
On 2016/05/06 15:25:48, rmcilroy wrote:
> This function is a bit hard for me to reason about (and will be even more
> complicated once we add things like merging bytecodes). I wonder if we could
> structure it something like this:
> 
> { 
>   UpdateCurrentBytecode(current);
> 
>   if (CanElideCurrent(current)) {
>     if (current->source_info().is_valid()) {
>       // <comment>
>       current->set_bytecode(Bytecode::kNop);
>     } else {
>       current->Release();
>       current = nullptr;
>     }
>     return;
>   }
>   
>   if (last_is_discardable_ && CanElideLast(current)) {
>     if (last_->source_info().is_valid()) {
>       current->source_info().Update(last_->source_info());
>     }
>     last_->Release();
>     last_ == nullptr;
>   }
>   return current;
> }
> 

Done.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:76: // can only be discarded if
last_is_discardable_ == true.
On 2016/05/06 15:25:48, rmcilroy wrote:
> This comment isn't really true given the source position requirements.

Done.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:79: current->set_bytecode(
On 2016/05/06 15:25:48, rmcilroy wrote:
> nit - could we have a replace_bytecode() in BytecodeNode which checks that a
> particular substitution is valid, and keeps operands / operand scales the
same?

Done.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:109:
!current->source_info().is_statement()) {
On 2016/05/06 15:25:48, rmcilroy wrote:
> It's a bit weird that this is the only is_statement() check. Shouldn't we just
> always never elide the last bytecode if both the last bytecode and the current
> bytecode have statement positions (otherwise we would swallow  a statement).
> This would probably be easier with the CanElideLast() function structure
> discussed above.

Cleaned-up in later CL. This is more oversight of not pushing Nop's through here
than anything else.

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

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.cc:18: entry.source_position() >
source_position())) {
On 2016/05/06 15:25:48, rmcilroy wrote:
> Could we get rid of the whole candidate_ source position entry logic in
> source-position-table-builder now that this is dealt with here?

Done.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.cc:94: void BytecodeNode::Print(std::ostream&
os) {
On 2016/05/06 15:25:48, rmcilroy wrote:
> Unused - is this useful for debugging?

Yes, I'd opt to keep this around for a while.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-writer.h (right):

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:23: class BytecodeSourceInfo final {
On 2016/05/06 15:25:48, rmcilroy wrote:
> Could we just use PositionTableEntry from source-position-table here instead
> please (and add any functions you need into that) and then we can modify
> SourcePositionTableBuilder to accept PositionTableEntry's instead of raw
> positions values.

Different layouts and sizes. Different semantics. Not keen on this suggestion.
Open to discuss offline.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:115: class BytecodeNodeAllocator final {
On 2016/05/06 15:25:48, rmcilroy wrote:
> I'd be really happy if we could avoid having to have a BytecodeNodeAllocator.
In
> this CL AFAIKT you are only really keeping a maximum of two BytecodeNodes
around
> (last_ in peephole and the current bytecode). Would there be a perf impact to
> simply copying the node in peephole optimizer (perhaps you could only keep
last_
> if there is a potential optimization you could do with that bytecode,
otherwise
> flush it through immediately)? My gut feeling is that the overhead of the
> copying will be offset by the fact that you no longer need to maintain the
> freelist. Not sure how this would play into the REO optimizer though.

As discussed, let's measure it and see.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:138: class BytecodeWriter {
On 2016/05/06 15:25:48, rmcilroy wrote:
> How about calling this BytecodePipelineStage, and renaming
> FinalStageBytecodeWriter as BytecodeArrayWriterStage or similar?

Done.

https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-writer.h:156: class FinalStageBytecodeWriter : public
BytecodeWriter {
On 2016/05/06 15:25:48, rmcilroy wrote:
> Nit - could we move this to it's own file.

Done.

[oth, 2016-05-09 15:03:56]

On 2016/05/09 11:02:17, oth wrote:
> Thanks for the feedback.
> 
> Incorporated nearly everything:
> 
> i) Checking out the perf implications of stack allocated BytecodeNodes is
TODO.
> ii) Not keen on unifying source position types.
> 
>
https://codereview.chromium.org/1947403002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/1947403002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecodes.h:256: /* eliminated bytecodess). */                
 
>                             \
> On 2016/05/05 21:15:07, oth wrote:
> > TODO(oth): typo
> 
> Done.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> File src/interpreter/bytecode-peephole-optimizer.cc (right):
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-peephole-optimizer.cc:72: BytecodeNode*
> BytecodePeepholeOptimizer::Optimize(BytecodeNode* current) {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > This function is a bit hard for me to reason about (and will be even more
> > complicated once we add things like merging bytecodes). I wonder if we could
> > structure it something like this:
> > 
> > { 
> >   UpdateCurrentBytecode(current);
> > 
> >   if (CanElideCurrent(current)) {
> >     if (current->source_info().is_valid()) {
> >       // <comment>
> >       current->set_bytecode(Bytecode::kNop);
> >     } else {
> >       current->Release();
> >       current = nullptr;
> >     }
> >     return;
> >   }
> >   
> >   if (last_is_discardable_ && CanElideLast(current)) {
> >     if (last_->source_info().is_valid()) {
> >       current->source_info().Update(last_->source_info());
> >     }
> >     last_->Release();
> >     last_ == nullptr;
> >   }
> >   return current;
> > }
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-peephole-optimizer.cc:76: // can only be discarded if
> last_is_discardable_ == true.
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > This comment isn't really true given the source position requirements.
> 
> Done.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-peephole-optimizer.cc:79: current->set_bytecode(
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > nit - could we have a replace_bytecode() in BytecodeNode which checks that a
> > particular substitution is valid, and keeps operands / operand scales the
> same?
> 
> Done.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-peephole-optimizer.cc:109:
> !current->source_info().is_statement()) {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > It's a bit weird that this is the only is_statement() check. Shouldn't we
just
> > always never elide the last bytecode if both the last bytecode and the
current
> > bytecode have statement positions (otherwise we would swallow  a statement).
> > This would probably be easier with the CanElideLast() function structure
> > discussed above.
> 
> Cleaned-up in later CL. This is more oversight of not pushing Nop's through
here
> than anything else.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> File src/interpreter/bytecode-writer.cc (right):
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-writer.cc:18: entry.source_position() >
> source_position())) {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > Could we get rid of the whole candidate_ source position entry logic in
> > source-position-table-builder now that this is dealt with here?
> 
> Done.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-writer.cc:94: void BytecodeNode::Print(std::ostream&
> os) {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > Unused - is this useful for debugging?
> 
> Yes, I'd opt to keep this around for a while.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> File src/interpreter/bytecode-writer.h (right):
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-writer.h:23: class BytecodeSourceInfo final {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > Could we just use PositionTableEntry from source-position-table here instead
> > please (and add any functions you need into that) and then we can modify
> > SourcePositionTableBuilder to accept PositionTableEntry's instead of raw
> > positions values.
> 
> Different layouts and sizes. Different semantics. Not keen on this suggestion.
> Open to discuss offline.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-writer.h:115: class BytecodeNodeAllocator final {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > I'd be really happy if we could avoid having to have a
BytecodeNodeAllocator.
> In
> > this CL AFAIKT you are only really keeping a maximum of two BytecodeNodes
> around
> > (last_ in peephole and the current bytecode). Would there be a perf impact
to
> > simply copying the node in peephole optimizer (perhaps you could only keep
> last_
> > if there is a potential optimization you could do with that bytecode,
> otherwise
> > flush it through immediately)? My gut feeling is that the overhead of the
> > copying will be offset by the fact that you no longer need to maintain the
> > freelist. Not sure how this would play into the REO optimizer though.
> 
> As discussed, let's measure it and see.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-writer.h:138: class BytecodeWriter {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > How about calling this BytecodePipelineStage, and renaming
> > FinalStageBytecodeWriter as BytecodeArrayWriterStage or similar?
> 
> Done.
> 
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-writer.h:156: class FinalStageBytecodeWriter : public
> BytecodeWriter {
> On 2016/05/06 15:25:48, rmcilroy wrote:
> > Nit - could we move this to it's own file.
> 
> Done.

JFYI, there is a change pending that removes the Nops preceding forward jumps in
the test. Having some kinks on the register optimizer with it that are being
resolved first.

[oth, 2016-05-09 15:44:09]

On 2016/05/09 15:03:56, oth wrote:
> On 2016/05/09 11:02:17, oth wrote:
> > Thanks for the feedback.
> > 
> > Incorporated nearly everything:
> > 
> > i) Checking out the perf implications of stack allocated BytecodeNodes is
> TODO.
> > ii) Not keen on unifying source position types.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/20001/src/interpreter/bytecod...
> > File src/interpreter/bytecodes.h (right):
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/20001/src/interpreter/bytecod...
> > src/interpreter/bytecodes.h:256: /* eliminated bytecodess). */              
 
>  
> >                             \
> > On 2016/05/05 21:15:07, oth wrote:
> > > TODO(oth): typo
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > File src/interpreter/bytecode-peephole-optimizer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-peephole-optimizer.cc:72: BytecodeNode*
> > BytecodePeepholeOptimizer::Optimize(BytecodeNode* current) {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > This function is a bit hard for me to reason about (and will be even more
> > > complicated once we add things like merging bytecodes). I wonder if we
could
> > > structure it something like this:
> > > 
> > > { 
> > >   UpdateCurrentBytecode(current);
> > > 
> > >   if (CanElideCurrent(current)) {
> > >     if (current->source_info().is_valid()) {
> > >       // <comment>
> > >       current->set_bytecode(Bytecode::kNop);
> > >     } else {
> > >       current->Release();
> > >       current = nullptr;
> > >     }
> > >     return;
> > >   }
> > >   
> > >   if (last_is_discardable_ && CanElideLast(current)) {
> > >     if (last_->source_info().is_valid()) {
> > >       current->source_info().Update(last_->source_info());
> > >     }
> > >     last_->Release();
> > >     last_ == nullptr;
> > >   }
> > >   return current;
> > > }
> > > 
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-peephole-optimizer.cc:76: // can only be discarded
if
> > last_is_discardable_ == true.
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > This comment isn't really true given the source position requirements.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-peephole-optimizer.cc:79: current->set_bytecode(
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > nit - could we have a replace_bytecode() in BytecodeNode which checks that
a
> > > particular substitution is valid, and keeps operands / operand scales the
> > same?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-peephole-optimizer.cc:109:
> > !current->source_info().is_statement()) {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > It's a bit weird that this is the only is_statement() check. Shouldn't we
> just
> > > always never elide the last bytecode if both the last bytecode and the
> current
> > > bytecode have statement positions (otherwise we would swallow  a
statement).
> > > This would probably be easier with the CanElideLast() function structure
> > > discussed above.
> > 
> > Cleaned-up in later CL. This is more oversight of not pushing Nop's through
> here
> > than anything else.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > File src/interpreter/bytecode-writer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-writer.cc:18: entry.source_position() >
> > source_position())) {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > Could we get rid of the whole candidate_ source position entry logic in
> > > source-position-table-builder now that this is dealt with here?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-writer.cc:94: void
BytecodeNode::Print(std::ostream&
> > os) {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > Unused - is this useful for debugging?
> > 
> > Yes, I'd opt to keep this around for a while.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > File src/interpreter/bytecode-writer.h (right):
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-writer.h:23: class BytecodeSourceInfo final {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > Could we just use PositionTableEntry from source-position-table here
instead
> > > please (and add any functions you need into that) and then we can modify
> > > SourcePositionTableBuilder to accept PositionTableEntry's instead of raw
> > > positions values.
> > 
> > Different layouts and sizes. Different semantics. Not keen on this
suggestion.
> > Open to discuss offline.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-writer.h:115: class BytecodeNodeAllocator final {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > I'd be really happy if we could avoid having to have a
> BytecodeNodeAllocator.
> > In
> > > this CL AFAIKT you are only really keeping a maximum of two BytecodeNodes
> > around
> > > (last_ in peephole and the current bytecode). Would there be a perf impact
> to
> > > simply copying the node in peephole optimizer (perhaps you could only keep
> > last_
> > > if there is a potential optimization you could do with that bytecode,
> > otherwise
> > > flush it through immediately)? My gut feeling is that the overhead of the
> > > copying will be offset by the fact that you no longer need to maintain the
> > > freelist. Not sure how this would play into the REO optimizer though.
> > 
> > As discussed, let's measure it and see.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-writer.h:138: class BytecodeWriter {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > How about calling this BytecodePipelineStage, and renaming
> > > FinalStageBytecodeWriter as BytecodeArrayWriterStage or similar?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1947403002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-writer.h:156: class FinalStageBytecodeWriter :
public
> > BytecodeWriter {
> > On 2016/05/06 15:25:48, rmcilroy wrote:
> > > Nit - could we move this to it's own file.
> > 
> > Done.
> 
> JFYI, there is a change pending that removes the Nops preceding forward jumps
in
> the test. Having some kinks on the register optimizer with it that are being
> resolved first.

Okay, resolved the issue with REO. The CL is clouded by a rebase against master
(deep sigh!). The pertinent change is to bytecode-array-builder in calculating
forward branches. FlushForOffset is now called after the jump has been emitted.

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

A few more comments, mostly nits, looks great overall.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:113: pipeline()->Write(current_node_);
I'm not super keen on the current_node_ field which is usually empty other than
potentially having a source position. If we end up dropping the bytecode node
allocator, could we instead just have a "pending_source_position_" field and
then only stack allocate the bytecode node in the output functions. 

It might actually make sense to pass the source position as a separate argument
through the pipeline, since it could end up getting associated with another
bytecode (e.g., this would avoid having to move the source position to a
different bytecode node in the peephole optimizer). WDYT?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:503: size_t current_offset =
pipeline()->FlushForOffset();
I'm a bit wary of this. You call this FlushForOffset, but you don't actually
flush the current bytecode node in the peephole optimizer. You then immediately
patch the jump if it is a forward target. If the last bytecode held in the
peephole optimizer is actually the bytecode which needs to be patched here then
the PatchJump call below will crash. I realize this shouldn't happen (a jump to
the immediate next bytecode) but I don't think we can guarantee it won't. WDYT?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:572: return jump_bytecode;
Why isn't this still UNREACHABLE?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:642: int delta =
static_cast<int>(jump_target - jump_location);
nit - personally I think it would be clearer to have the FlushForOffset here in
PatchJump (instead of Label:Bind) to match with OutputJump (which would also
probably mean getting the bytecodes vector here instead of in Label:Bind).

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:682: jump_bytecode =
GetJumpWithToBoolean(jump_bytecode);
We should just explicitly always call OutputJump with the ToBoolean variant and
strip it out in the peephole optimizer if possible.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:686: size_t current_offset =
pipeline()->FlushForOffset();
nit - move FlushForOffset out of the if / else branch.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:135: if (count == 1 &&
nit - how about:

if (++operand_index < Bytecodes::NumberOfOperands(current_bytecode()) &&
    Bytecodes::GetOperandType(current_bytecode(), operand_index) ==
        OperandType::kRegCount) {
  DCHECK_EQ(1, Bytecodes::GetNumberOfRegistersRepresentedBy(operand_type));
  return GetRegisterCountOperand(operand_index);
} else {
  return Bytecodes::GetNumberOfRegistersRepresentedBy(operand_type);
}

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:41:
source_position_table_builder_->AddStatementPosition(
nit - maybe just update SourcePositionTableBuilder to have one
AddPosition(size_t bytecode_offset, int source_position, bool is_statemtent)?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:96: frame_register_count_ =
Nit - max_register_count_ ?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:105: int
BytecodeArrayWriter::GetMeasuredFrameSize() {
nit - GetMaxRegisterCount() ?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:40: void
BytecodePeepholeOptimizer::LeaveBasicBlock() {
How about FlushForNewBasicBlock()?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:76: Bytecode jump =
Bytecodes::GetJumpWithoutToBoolean(current->bytecode());
nit - comment.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:84: last_->bytecode() ==
Bytecode::kStar) &&
nit - add Bytecodes::IsLdarOrStar() helper

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:88: return true;
Comment on all branches with reasoning behind decision.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:93: last_->bytecode() ==
Bytecode::kToName)) {
This if check is pretty difficult to parse. Could you add a helper for
BytecodeWritesNameToAccumulator(last_) which is structured similarly to
BytecodeArrayBuilder::CastAccumulatorToName (e.g., switching on the bytecode).

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:104: return false;
Comment on all branches with reasoning behind decision.

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

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.cc:89: Bytecodes::GetOperandType(bytecode_,
0)) {
I think we should only do these checks as DCHECKS - e.g., have: 
 DCHECK(Bytecodes::CanReplace(bytecode, bytecode_));
 bytecode_ = bytecode;

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:18: class TemporaryRegisterAllocator;
All these forward declarations seem to be unused.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:141: // Interface for bytecode writers.
/bytecode writers/bytecode writer pipeline stages/

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:290: kLast = (1
OPERAND_SCALE_LIST(COUNT_OPERAND_SCALES)) / 2
Do we really need kLast? It is only used once to check OperandScale::kQuadruple
== OperandScale::kLast so I'm not sure it's worth the complexity of this macro.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:481: static bool
WritesAccumulatorWithBoolean(Bytecode bytecode);
nit - WritesBooleanToAccumulator

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
src/interpreter/source-position-table.cc:131: if (entry.bytecode_offset ==
kUninitializedCandidateOffset) return;
You can remove this line

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
src/interpreter/source-position-table.h:53: static const int
kUninitializedCandidateOffset = -1;
You can remove this.

[oth, 2016-05-11 13:14:30]

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

[commit-bot: I haz the power, 2016-05-11 13:14:40]

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

[oth, 2016-05-11 13:17:31]

Thanks for the feedback, nearly everything incorporated.

No rush to make another pass just now as there's a need to write the
corresponding tests.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:113: pipeline()->Write(current_node_);
On 2016/05/10 11:14:09, rmcilroy wrote:
> I'm not super keen on the current_node_ field which is usually empty other
than
> potentially having a source position. If we end up dropping the bytecode node
> allocator, could we instead just have a "pending_source_position_" field and
> then only stack allocate the bytecode node in the output functions. 

Done. And measured and moved over to stack allocated bytecode nodes.

> It might actually make sense to pass the source position as a separate
argument
> through the pipeline, since it could end up getting associated with another
> bytecode (e.g., this would avoid having to move the source position to a
> different bytecode node in the peephole optimizer). WDYT?

For ease of reasoning I'd prefer to keep the two connected (at least for now).

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:503: size_t current_offset =
pipeline()->FlushForOffset();
On 2016/05/10 11:14:09, rmcilroy wrote:
> I'm a bit wary of this. You call this FlushForOffset, but you don't actually
> flush the current bytecode node in the peephole optimizer. You then
immediately
> patch the jump if it is a forward target. If the last bytecode held in the
> peephole optimizer is actually the bytecode which needs to be patched here
then
> the PatchJump call below will crash. I realize this shouldn't happen (a jump
to
> the immediate next bytecode) but I don't think we can guarantee it won't.
WDYT?

Emitting a jump entails writing the jump into the pipeline and invoking
LeaveBasicBlock which flushes the jump bytecode. So the crash/error described
does not occur. We definitely see generated bytecode that jumps to the next
bytecode. An example is in the semantics of Continue changing to jump to the
loop tail that came with generators.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:572: return jump_bytecode;
On 2016/05/10 11:14:09, rmcilroy wrote:
> Why isn't this still UNREACHABLE? 

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:642: int delta =
static_cast<int>(jump_target - jump_location);
On 2016/05/10 11:14:09, rmcilroy wrote:
> nit - personally I think it would be clearer to have the FlushForOffset here
in
> PatchJump (instead of Label:Bind) to match with OutputJump (which would also
> probably mean getting the bytecodes vector here instead of in Label:Bind).

There is no problem getting the vector here => Done.

FlushForOffset needs to be unconditionally called in Bind(label) whereas
PatchJump is only called for forward jumps. It looks like Bind(target, label)
does not need FlushForOffset at all since the pipeline would havre been flushed
when the position of target was bound.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:682: jump_bytecode =
GetJumpWithToBoolean(jump_bytecode);
On 2016/05/10 11:14:09, rmcilroy wrote:
> We should just explicitly always call OutputJump with the ToBoolean variant
and
> strip it out in the peephole optimizer if possible.

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:686: size_t current_offset =
pipeline()->FlushForOffset();
On 2016/05/10 11:14:09, rmcilroy wrote:
> nit - move FlushForOffset out of the if / else branch.

Not possible in the latest revision as the FlushForOffset is moved after
OutputScaled in the second branch together with some additional logic in the
peephole optimizer to remove the Nop's before forward branches.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:135: if (count == 1 &&
On 2016/05/10 11:14:09, rmcilroy wrote:
> nit - how about:
> 
> if (++operand_index < Bytecodes::NumberOfOperands(current_bytecode()) &&
>     Bytecodes::GetOperandType(current_bytecode(), operand_index) ==
>         OperandType::kRegCount) {
>   DCHECK_EQ(1, Bytecodes::GetNumberOfRegistersRepresentedBy(operand_type));
>   return GetRegisterCountOperand(operand_index);
> } else {
>   return Bytecodes::GetNumberOfRegistersRepresentedBy(operand_type);
> }

Updated to use GetOperandTypes instead.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:41:
source_position_table_builder_->AddStatementPosition(
On 2016/05/10 11:14:09, rmcilroy wrote:
> nit - maybe just update SourcePositionTableBuilder to have one
> AddPosition(size_t bytecode_offset, int source_position, bool is_statemtent)?

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:96: frame_register_count_ =
On 2016/05/10 11:14:09, rmcilroy wrote:
> Nit - max_register_count_ ?

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:105: int
BytecodeArrayWriter::GetMeasuredFrameSize() {
On 2016/05/10 11:14:09, rmcilroy wrote:
> nit - GetMaxRegisterCount() ?

The emphasis is on measured. Returning the size rather than the count keeps the
point of use relatively clean.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:40: void
BytecodePeepholeOptimizer::LeaveBasicBlock() {
On 2016/05/10 11:14:09, rmcilroy wrote:
> How about FlushForNewBasicBlock()?

I hate to bikeshed, but it really is used at the end of a basic block.
Renamed FlushBasicBlock().

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:76: Bytecode jump =
Bytecodes::GetJumpWithoutToBoolean(current->bytecode());
On 2016/05/10 11:14:09, rmcilroy wrote:
> nit - comment.

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:84: last_->bytecode() ==
Bytecode::kStar) &&
On 2016/05/10 11:14:10, rmcilroy wrote:
> nit - add Bytecodes::IsLdarOrStar() helper

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:88: return true;
On 2016/05/10 11:14:09, rmcilroy wrote:
> Comment on all branches with reasoning behind decision.

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:93: last_->bytecode() ==
Bytecode::kToName)) {
On 2016/05/10 11:14:10, rmcilroy wrote:
> This if check is pretty difficult to parse. Could you add a helper for
> BytecodeWritesNameToAccumulator(last_) which is structured similarly to
> BytecodeArrayBuilder::CastAccumulatorToName (e.g., switching on the bytecode).


Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:104: return false;
On 2016/05/10 11:14:10, rmcilroy wrote:
> Comment on all branches with reasoning behind decision.

Done.

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

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.cc:89: Bytecodes::GetOperandType(bytecode_,
0)) {
On 2016/05/10 11:14:10, rmcilroy wrote:
> I think we should only do these checks as DCHECKS - e.g., have: 
>  DCHECK(Bytecodes::CanReplace(bytecode, bytecode_));
>  bytecode_ = bytecode;

The semantics of CanReplace will be too cryptic away from point of use. It could
mean a variety of things - the optimizer here is doing a variety of
"replacements".

I have removed this method. The points of use for set_bytecode ensure the
correct number of arguments and operand_scale. This is not something a wily
developer accidentally calls.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:18: class TemporaryRegisterAllocator;
On 2016/05/10 11:14:10, rmcilroy wrote:
> All these forward declarations seem to be unused.

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:141: // Interface for bytecode writers.
On 2016/05/10 11:14:10, rmcilroy wrote:
> /bytecode writers/bytecode writer pipeline stages/

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:290: kLast = (1
OPERAND_SCALE_LIST(COUNT_OPERAND_SCALES)) / 2
On 2016/05/10 11:14:10, rmcilroy wrote:
> Do we really need kLast? It is only used once to check
OperandScale::kQuadruple
> == OperandScale::kLast so I'm not sure it's worth the complexity of this
macro.

It ensures code does not read uninitialised memory at the point of use. The code
is changed so kLast is manually added to the end of the list. This has other
problems, but at least it avoids reading invalid memory.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:481: static bool
WritesAccumulatorWithBoolean(Bytecode bytecode);
On 2016/05/10 11:14:10, rmcilroy wrote:
> nit - WritesBooleanToAccumulator

How about WritesAnyPossibleBooleanValueToTheAccumulator?

Done (the shorter :-).

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
src/interpreter/source-position-table.cc:131: if (entry.bytecode_offset ==
kUninitializedCandidateOffset) return;
On 2016/05/10 11:14:10, rmcilroy wrote:
> You can remove this line

Done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/source...
src/interpreter/source-position-table.h:53: static const int
kUninitializedCandidateOffset = -1;
On 2016/05/10 11:14:10, rmcilroy wrote:
> You can remove this.

Done.

[commit-bot: I haz the power, 2016-05-11 13:44:57]

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

[commit-bot: I haz the power, 2016-05-11 13:44:58]

Dry run: This issue passed the CQ dry run.

[oth, 2016-05-12 11:41:05]

oth@chromium.org changed reviewers:
+ mstarzinger@chromium.org, yangguo@chromium.org

[oth, 2016-05-12 11:41:05]

Yang, PTAL at source-position-table.{h,cc}. The filtering of source positions is
moved into BytecodeSourceInfo::Update() in bytecode-node.h.

Michi, there's a trivial compiler change here.

Thanks

[Yang, 2016-05-12 12:03:52]

lgtm

[rmcilroy, 2016-05-12 12:15:14]

LGTM with some last nits.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:105: int
BytecodeArrayWriter::GetMeasuredFrameSize() {
On 2016/05/11 13:17:30, oth wrote:
> On 2016/05/10 11:14:09, rmcilroy wrote:
> > nit - GetMaxRegisterCount() ?
> 
> The emphasis is on measured. Returning the size rather than the count keeps
the
> point of use relatively clean.

How about GetMaximumFrameSizeUsed()?

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:40: void
BytecodePeepholeOptimizer::LeaveBasicBlock() {
On 2016/05/11 13:17:30, oth wrote:
> On 2016/05/10 11:14:09, rmcilroy wrote:
> > How about FlushForNewBasicBlock()?
> 
> I hate to bikeshed, but it really is used at the end of a basic block.
> Renamed FlushBasicBlock().

Works for me, thanks!

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

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.cc:89: Bytecodes::GetOperandType(bytecode_,
0)) {
On 2016/05/11 13:17:31, oth wrote:
> On 2016/05/10 11:14:10, rmcilroy wrote:
> > I think we should only do these checks as DCHECKS - e.g., have: 
> >  DCHECK(Bytecodes::CanReplace(bytecode, bytecode_));
> >  bytecode_ = bytecode;
> 
> The semantics of CanReplace will be too cryptic away from point of use. It
could
> mean a variety of things - the optimizer here is doing a variety of
> "replacements".
> 
> I have removed this method. The points of use for set_bytecode ensure the
> correct number of arguments and operand_scale. This is not something a wily
> developer accidentally calls.

Works for me.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:290: kLast = (1
OPERAND_SCALE_LIST(COUNT_OPERAND_SCALES)) / 2
On 2016/05/11 13:17:31, oth wrote:
> On 2016/05/10 11:14:10, rmcilroy wrote:
> > Do we really need kLast? It is only used once to check
> OperandScale::kQuadruple
> > == OperandScale::kLast so I'm not sure it's worth the complexity of this
> macro.
> 
> It ensures code does not read uninitialised memory at the point of use. The
code
> is changed so kLast is manually added to the end of the list. This has other
> problems, but at least it avoids reading invalid memory.

Acknowledged.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:114: }
nit - add a helper AttachSourceInfo(BytecodeNode* node) and use it in all the
Output functions.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:508: // a number.
Nit - update comment or move to peephole optimizer.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:702: // Calculate label position after
flushing for offset.
nit - newline above and "// Calculate the label position by flushing for offset
after emitting the jump bytecode."

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:20: last_(Bytecode::kNop),
nit - as mentioned offline, would be nice to use kInvalid as invalid bytecode
(and get rid of last_is_valid_ field) once you have addressed the TODO for never
emiting kInvalid. Don't block this CL on that though.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:39:
!last_.source_info().is_statement()) {
nit - please add a comment here as to why the Nop can be invalidated if it
doesn't have a statement position.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:64: // Only output if
optimization did not invalidate earlier node.
nit - // Only output the last node if it wasn't invalidated by the optimization.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:89:
GetConstantForIndexOperand(&last_, 0)->IsName()));
I wish we could put this in bytecodes.h but don't see an easy way to do it with
having to get a constant from the constant pool :(.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:112: // of back-to-back of Ldar
and Star bytecodes with the same operand.
nit - remove second "of"

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.h (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:16: class
BytecodePeepholeOptimizer : public BytecodePipelineStage,
nit - add class comment

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:30: BytecodeNode*
Optimize(BytecodeNode* current);
nit - move Optimize up top and put a newline between it and the other helpers.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.cc (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.cc:23: // statement in a function.
I'm not sure what "This last piece is needed for the first statement in a
function." means?

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:16: class BytecodeNodeAllocator;
Remove

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:108: class BytecodePipelineStage {
nit - Forward declare BytecodeNode and BytecodeSourceInfo and put
BytecodePipelineStage as the first class in the header.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/source...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/source...
src/interpreter/source-position-table.h:49: void AddExpressionPosition(size_t
bytecode_offset, int source_position);
Remove these two functions, I don't think they are used any longer (except for
tests which should be updated to use the one below)

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-array-writer-unittest.cc (right):

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-writer-unittest.cc:91: // clang-format
off
nit - indent

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-writer-unittest.cc:116: //
clang-format on
nit - indent

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-writer-unittest.cc:123: #define
NODE(node_args)          \
Nit - these could be helper functions instead of macros (I think that would be
clearer).

[oth, 2016-05-12 14:59:53]

Thanks, all done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:114: }
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - add a helper AttachSourceInfo(BytecodeNode* node) and use it in all the
> Output functions.

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:508: // a number.
On 2016/05/12 12:15:13, rmcilroy wrote:
> Nit - update comment or move to peephole optimizer.

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:702: // Calculate label position after
flushing for offset.
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - newline above and "// Calculate the label position by flushing for
offset
> after emitting the jump bytecode."

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:20: last_(Bytecode::kNop),
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - as mentioned offline, would be nice to use kInvalid as invalid bytecode
> (and get rid of last_is_valid_ field) once you have addressed the TODO for
never
> emiting kInvalid. Don't block this CL on that though.

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:39:
!last_.source_info().is_statement()) {
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - please add a comment here as to why the Nop can be invalidated if it
> doesn't have a statement position.

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:64: // Only output if
optimization did not invalidate earlier node.
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - // Only output the last node if it wasn't invalidated by the
optimization.

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:89:
GetConstantForIndexOperand(&last_, 0)->IsName()));
On 2016/05/12 12:15:13, rmcilroy wrote:
> I wish we could put this in bytecodes.h but don't see an easy way to do it
with
> having to get a constant from the constant pool :(.

Acknowledged.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:112: // of back-to-back of Ldar
and Star bytecodes with the same operand.
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - remove second "of"

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.h (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:16: class
BytecodePeepholeOptimizer : public BytecodePipelineStage,
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - add class comment

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:30: BytecodeNode*
Optimize(BytecodeNode* current);
On 2016/05/12 12:15:13, rmcilroy wrote:
> nit - move Optimize up top and put a newline between it and the other helpers.

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.cc (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.cc:23: // statement in a function.
On 2016/05/12 12:15:13, rmcilroy wrote:
> I'm not sure what "This last piece is needed for the first statement in a
> function." means?

Comment updated. IIRC there may be multiple statement positions emitted before
the first bytecode is emitted. The generated bytecode always wants the latest.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-pipeline.h (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:16: class BytecodeNodeAllocator;
On 2016/05/12 12:15:13, rmcilroy wrote:
> Remove

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-pipeline.h:108: class BytecodePipelineStage {
On 2016/05/12 12:15:14, rmcilroy wrote:
> nit - Forward declare BytecodeNode and BytecodeSourceInfo and put
> BytecodePipelineStage as the first class in the header.

Done.

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/source...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1947403002/diff/140001/src/interpreter/source...
src/interpreter/source-position-table.h:49: void AddExpressionPosition(size_t
bytecode_offset, int source_position);
On 2016/05/12 12:15:14, rmcilroy wrote:
> Remove these two functions, I don't think they are used any longer (except for
> tests which should be updated to use the one below)

Done.

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-array-writer-unittest.cc (right):

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-writer-unittest.cc:91: // clang-format
off
On 2016/05/12 12:15:14, rmcilroy wrote:
> nit - indent

Done.

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-writer-unittest.cc:116: //
clang-format on
On 2016/05/12 12:15:14, rmcilroy wrote:
> nit - indent

Done.

https://codereview.chromium.org/1947403002/diff/180001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-writer-unittest.cc:123: #define
NODE(node_args)          \
On 2016/05/12 12:15:14, rmcilroy wrote:
> Nit - these could be helper functions instead of macros (I think that would be
> clearer).

Done.

[rmcilroy, 2016-05-12 15:19:47]

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:105: int
BytecodeArrayWriter::GetMeasuredFrameSize() {
On 2016/05/12 12:15:13, rmcilroy wrote:
> On 2016/05/11 13:17:30, oth wrote:
> > On 2016/05/10 11:14:09, rmcilroy wrote:
> > > nit - GetMaxRegisterCount() ?
> > 
> > The emphasis is on measured. Returning the size rather than the count keeps
> the
> > point of use relatively clean.
> 
> How about GetMaximumFrameSizeUsed()?

Did you miss this comment, or prefer GetMeasuredFrameSize()?

[oth, 2016-05-12 15:35:28]

On 2016/05/12 15:19:47, rmcilroy wrote:
>
https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
> File src/interpreter/bytecode-array-writer.cc (right):
> 
>
https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-array-writer.cc:105: int
> BytecodeArrayWriter::GetMeasuredFrameSize() {
> On 2016/05/12 12:15:13, rmcilroy wrote:
> > On 2016/05/11 13:17:30, oth wrote:
> > > On 2016/05/10 11:14:09, rmcilroy wrote:
> > > > nit - GetMaxRegisterCount() ?
> > > 
> > > The emphasis is on measured. Returning the size rather than the count
keeps
> > the
> > > point of use relatively clean.
> > 
> > How about GetMaximumFrameSizeUsed()?
> 
> Did you miss this comment, or prefer GetMeasuredFrameSize()?

Yes, will fix.

[oth, 2016-05-12 15:44:31]

Thanks, done.

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-writer.cc (right):

https://codereview.chromium.org/1947403002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-writer.cc:105: int
BytecodeArrayWriter::GetMeasuredFrameSize() {
On 2016/05/12 15:19:47, rmcilroy wrote:
> On 2016/05/12 12:15:13, rmcilroy wrote:
> > On 2016/05/11 13:17:30, oth wrote:
> > > On 2016/05/10 11:14:09, rmcilroy wrote:
> > > > nit - GetMaxRegisterCount() ?
> > > 
> > > The emphasis is on measured. Returning the size rather than the count
keeps
> > the
> > > point of use relatively clean.
> > 
> > How about GetMaximumFrameSizeUsed()?
> 
> Did you miss this comment, or prefer GetMeasuredFrameSize()?

Done.

[Michael Starzinger, 2016-05-12 16:06:46]

LGTM on "compiler", I did't look at the rest (yet). Let me know if I should
review the rest. But also feel free to land right away. Up to you essentially.

[oth, 2016-05-12 18:12:53]

On 2016/05/12 16:06:46, Michael Starzinger wrote:
> LGTM on "compiler", I did't look at the rest (yet). Let me know if I should
> review the rest. But also feel free to land right away. Up to you essentially.

Thanks Michi.

It'd be good to get your feedback if you have time to look over the beast in
it's entirety. I'd like to land it to avoid blocking, but will gladly
incorporate feedback that arises. Follow ons from this are creating fused
bytecodes and the register optimizer.

Best, O.

[oth, 2016-05-12 18:44:58]

Description was changed from

==========
[interpreter] Introduce bytecode generation pipeline.

This change introduces a pipeline for the final stages of
bytecode generation.

The peephole optimizer is made distinct from the BytecodeArrayBuilder.

A new FinalStageBytecodeWriter is responsible for writing bytecode. It
also keeps track of the maximum register seen and offers a potentially
smaller frame size.

R=rmcilroy@chromium.org
LOG=N
BUG=v8:4280
==========

to

==========
[interpreter] Introduce bytecode generation pipeline.

This change introduces a pipeline for the final stages of
bytecode generation.

The peephole optimizer is made distinct from the BytecodeArrayBuilder.

A new BytecodeArrayWriter is responsible for writing bytecode. It
also keeps track of the maximum register seen and offers a potentially
smaller frame size.

R=rmcilroy@chromium.org
LOG=N
BUG=v8:4280
==========

[oth, 2016-05-12 18:45:56]

The CQ bit was checked by oth@chromium.org

[oth, 2016-05-12 18:45:56]

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

[commit-bot: I haz the power, 2016-05-12 18:46:08]

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

[commit-bot: I haz the power, 2016-05-12 19:18:16]

Description was changed from

==========
[interpreter] Introduce bytecode generation pipeline.

This change introduces a pipeline for the final stages of
bytecode generation.

The peephole optimizer is made distinct from the BytecodeArrayBuilder.

A new BytecodeArrayWriter is responsible for writing bytecode. It
also keeps track of the maximum register seen and offers a potentially
smaller frame size.

R=rmcilroy@chromium.org
LOG=N
BUG=v8:4280
==========

to

==========
[interpreter] Introduce bytecode generation pipeline.

This change introduces a pipeline for the final stages of
bytecode generation.

The peephole optimizer is made distinct from the BytecodeArrayBuilder.

A new BytecodeArrayWriter is responsible for writing bytecode. It
also keeps track of the maximum register seen and offers a potentially
smaller frame size.

R=rmcilroy@chromium.org
LOG=N
BUG=v8:4280
==========

[commit-bot: I haz the power, 2016-05-12 19:18:17]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-05-12 19:20:14]

Description was changed from

==========
[interpreter] Introduce bytecode generation pipeline.

This change introduces a pipeline for the final stages of
bytecode generation.

The peephole optimizer is made distinct from the BytecodeArrayBuilder.

A new BytecodeArrayWriter is responsible for writing bytecode. It
also keeps track of the maximum register seen and offers a potentially
smaller frame size.

R=rmcilroy@chromium.org
LOG=N
BUG=v8:4280
==========

to

==========
[interpreter] Introduce bytecode generation pipeline.

This change introduces a pipeline for the final stages of
bytecode generation.

The peephole optimizer is made distinct from the BytecodeArrayBuilder.

A new BytecodeArrayWriter is responsible for writing bytecode. It
also keeps track of the maximum register seen and offers a potentially
smaller frame size.

R=rmcilroy@chromium.org
LOG=N
BUG=v8:4280

Committed: https://crrev.com/02b7373ab135a516c4f553a2f553d086d4c12579
Cr-Commit-Position: refs/heads/master@{#36220}
==========

[commit-bot: I haz the power, 2016-05-12 19:20:16]

Patchset 14 (id:??) landed as
https://crrev.com/02b7373ab135a516c4f553a2f553d086d4c12579
Cr-Commit-Position: refs/heads/master@{#36220}