[interpeter] Move to table based peephole optimizer.

[interpeter] Move to table based peephole optimizer.

Introduces a lookup table for peephole optimizations.

Fixes some tests using BytecodePeepholeOptimizer::Write() that should
have been update to use BytecodePeepholeOptimizer::WriteJump().

BUG=v8:4280
LOG=N

Committed: https://crrev.com/f4234422b93b21a286b0f31799009bcbe8b90b9e
Cr-Commit-Position: refs/heads/master@{#37819}

[oth, 2016-07-04 10:28:38]

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

[oth, 2016-07-04 10:28:39]

Ross, PTAL, fewer branches when writing bytecode. The LOC ends up a bit larger
and no doubt some of the names will grate. Perf seems to be largely neutral.

[rmcilroy, 2016-07-04 12:45:46]

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

https://codereview.chromium.org/2118183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:216:
PeepholeActionTable::Initialize();
How does this play with threading? There could be multiple isolates initializing
at the same time on different threads, wouldn't this be a race? Maybe make this
per-isolate (or even better, make it at compile time if possible?)

https://codereview.chromium.org/2118183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:468:
PeepholeActionTable::Lookup(last()->bytecode(), node->bytecode());
You could possibly store function pointers in the table to avoid this switch,
WDYT?

[oth, 2016-07-04 13:27:29]

On 2016/07/04 12:45:46, rmcilroy wrote:
>
https://codereview.chromium.org/2118183002/diff/80001/src/interpreter/bytecod...
> File src/interpreter/bytecode-peephole-optimizer.cc (right):
> 
>
https://codereview.chromium.org/2118183002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-peephole-optimizer.cc:216:
> PeepholeActionTable::Initialize();
> How does this play with threading? There could be multiple isolates
initializing
> at the same time on different threads, wouldn't this be a race? Maybe make
this
> per-isolate (or even better, make it at compile time if possible?)

Changed to use base::CallOnce.

Compile time initialization is left as a TODO as it's non-trivial, but worthy of
investigation. It possibly looks like a set of constexpr methods for evaluating
bytecode properties or template specializations to the same end.

>
https://codereview.chromium.org/2118183002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-peephole-optimizer.cc:468:
> PeepholeActionTable::Lookup(last()->bytecode(), node->bytecode());
> You could possibly store function pointers in the table to avoid this switch,
> WDYT?

It performs worse. The action ends up 4x or 8x larger and reduces cache
efficiency.

[oth, 2016-07-08 07:39:37]

Ross, PTAL.

The CL now builds the table at compile time via a little helper program
mkpeephole.

gn build to follow.

Thanks

[rmcilroy, 2016-07-08 13:18:25]

Haven't looked in huge detail yet, but worried about build dependencies.

https://codereview.chromium.org/2118183002/diff/340001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2118183002/diff/340001/BUILD.gn#newcode1312
BUILD.gn:1312: "$target_gen_dir/bytecode-peephole-table.h",
I think this should be a separate target which needs to be built before trying
to build the rest of v8_base (otherwise bytecode-peephole-optimizer.cc could be
built before you build or rebuild bytecode-peephole-table.h, thus meaning it
would be built with the wrong table (or the build would break due to a missing
header). 

Thinking about it, could you make this a cc file and just rely on linking to the
table with a known name? That would mean you could make only the v8_*_snapshot
targets depend on this new target (in the same way as they depend on js2c).

https://codereview.chromium.org/2118183002/diff/340001/src/interpreter/mkpeep...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2118183002/diff/340001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:81: PeepholeActionAndData
PeepholeActionTableWriter::LookupActionAndData(
nit - it might be nice if this was in a seperate file (e.g.,
bytecode-peephole-optimizations.h/cc) so that it is seperate from the mechanics
of building the actual file (and is more obviously related to
bytecode-peephole-optimizer.cc). WDYT?

https://codereview.chromium.org/2118183002/diff/340001/src/v8.gyp
File src/v8.gyp (right):

https://codereview.chromium.org/2118183002/diff/340001/src/v8.gyp#newcode927
src/v8.gyp:927: '<(INTERMEDIATE_DIR)/bytecode-peephole-table.h',
Same comment as gn file.

[oth, 2016-07-08 15:12:34]

Thanks.

https://codereview.chromium.org/2118183002/diff/340001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2118183002/diff/340001/BUILD.gn#newcode1312
BUILD.gn:1312: "$target_gen_dir/bytecode-peephole-table.h",
On 2016/07/08 13:18:25, rmcilroy wrote:
> I think this should be a separate target which needs to be built before trying
> to build the rest of v8_base (otherwise bytecode-peephole-optimizer.cc could
be
> built before you build or rebuild bytecode-peephole-table.h, thus meaning it
> would be built with the wrong table (or the build would break due to a missing
> header). 

I'm not a GN expert, but it definitely detects if there is not action that
outputs this header and refuses to build. It knows that is a dependency and
builds it. I believe it's down to the specific value of the prefix held in
$target_gen_dir.

> Thinking about it, could you make this a cc file and just rely on linking to
the
> table with a known name? That would mean you could make only the v8_*_snapshot
> targets depend on this new target (in the same way as they depend on js2c).

What happens in this path is rather convoluted. Okay, let's try it.

https://codereview.chromium.org/2118183002/diff/340001/src/interpreter/mkpeep...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2118183002/diff/340001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:81: PeepholeActionAndData
PeepholeActionTableWriter::LookupActionAndData(
On 2016/07/08 13:18:25, rmcilroy wrote:
> nit - it might be nice if this was in a seperate file (e.g.,
> bytecode-peephole-optimizations.h/cc) so that it is seperate from the
mechanics
> of building the actual file (and is more obviously related to
> bytecode-peephole-optimizer.cc). WDYT?

WITI this has more scope to go wrong and make work should there be any updates.
Trying this route for the gcmole issue.

https://codereview.chromium.org/2118183002/diff/340001/src/v8.gyp
File src/v8.gyp (right):

https://codereview.chromium.org/2118183002/diff/340001/src/v8.gyp#newcode927
src/v8.gyp:927: '<(INTERMEDIATE_DIR)/bytecode-peephole-table.h',
On 2016/07/08 13:18:25, rmcilroy wrote:
> Same comment as gn file.

Here there are explicit dependencies on the generation of the file. So part A of
your comment is again mistaken.

Same comment for part B. I suspect we have to do as you suggest for the gcmole
build. I don't see conflating this with the snapshot path as having any other
benefit.

[Michael Achenbach, 2016-07-11 09:11:41]

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

[Michael Achenbach, 2016-07-11 09:11:43]

https://codereview.chromium.org/2118183002/diff/420001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2118183002/diff/420001/BUILD.gn#newcode1899
BUILD.gn:1899: "//:run_mkpeephole",
The // resolve to the project root. That means that when embedded into chromium
it'll be src and not src/v8. Like that it'd need a run_mkpeephole target in
chromium as well.

[Michael Achenbach, 2016-07-11 09:18:48]

Please manually select other chromium trybots for larger gn changes, because our
gn coverage isn't all that good yet.

E.g.
android_arm64_dbg_recipe
chromeos_daisy_chromium_compile_only_ng
linux_chromium_compile_dbg_ng

You can find them when clicking on "Choose trybots".

[oth, 2016-07-11 09:20:34]

On 2016/07/11 09:11:43, Michael Achenbach wrote:
> https://codereview.chromium.org/2118183002/diff/420001/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/2118183002/diff/420001/BUILD.gn#newcode1899
> BUILD.gn:1899: "//:run_mkpeephole",
> The // resolve to the project root. That means that when embedded into
chromium
> it'll be src and not src/v8. Like that it'd need a run_mkpeephole target in
> chromium as well.

Thanks, Michael. Trying this now.

[Michael Achenbach, 2016-07-11 09:35:45]

I added a few more chromium trybots.

[oth, 2016-07-11 10:45:53]

On 2016/07/11 09:35:45, Michael Achenbach wrote:
> I added a few more chromium trybots.

Thanks Michael. A couple of these have highlighted some further issues to work
through. To run win_clang and android_arm64_dbg_recipe builds, do I need special
permissions to do this?

Trying:
  git cl try -b android_arm64_debug_recipe -m tryserver.chromium.linux

Yields a purple highlight in the UI, with an error buildbucket 9....

[Michael Achenbach, 2016-07-11 10:51:34]

On 2016/07/11 10:45:53, oth wrote:
> On 2016/07/11 09:35:45, Michael Achenbach wrote:
> > I added a few more chromium trybots.
> 
> Thanks Michael. A couple of these have highlighted some further issues to work
> through. To run win_clang and android_arm64_dbg_recipe builds, do I need
special
> permissions to do this?
> 
> Trying:
>   git cl try -b android_arm64_debug_recipe -m tryserver.chromium.linux
> 
> Yields a purple highlight in the UI, with an error buildbucket 9....

Just don't specify the tryserver. It should infer it in most cases. In your call
above, I think the tryserver should be tryserver.chromium.android. I often
choose them in the web-ui. There you can also see the tryserver-trybot mapping.
'git cl try' also only supports adding bots from the same tryserver in one call.

Ping me if it still fails.

[Michael Achenbach, 2016-07-11 10:54:07]

For repairing android and maybe windows...

https://codereview.chromium.org/2118183002/diff/440001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2118183002/diff/440001/BUILD.gn#newcode2140
BUILD.gn:2140: configs = [ ":internal_config" ]
You also need ":external_config" like in other executables below for the android
error.

[Michael Achenbach, 2016-07-11 14:08:40]

https://codereview.chromium.org/2118183002/diff/440001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2118183002/diff/440001/BUILD.gn#newcode2143
BUILD.gn:2143: ":v8_libbase",
also "//build/win:default_exe_manifest" should be added to deps.

[oth, 2016-07-12 10:08:53]

Thanks Michael, greatly appreciated.

https://codereview.chromium.org/2118183002/diff/420001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2118183002/diff/420001/BUILD.gn#newcode1899
BUILD.gn:1899: "//:run_mkpeephole",
On 2016/07/11 09:11:42, Michael Achenbach wrote:
> The // resolve to the project root. That means that when embedded into
chromium
> it'll be src and not src/v8. Like that it'd need a run_mkpeephole target in
> chromium as well.

Ack, yes, done.

https://codereview.chromium.org/2118183002/diff/440001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2118183002/diff/440001/BUILD.gn#newcode2140
BUILD.gn:2140: configs = [ ":internal_config" ]
On 2016/07/11 10:54:07, Michael Achenbach wrote:
> You also need ":external_config" like in other executables below for the
android
> error.

Ack, done. Thanks!

https://codereview.chromium.org/2118183002/diff/440001/BUILD.gn#newcode2143
BUILD.gn:2143: ":v8_libbase",
On 2016/07/11 14:08:40, Michael Achenbach wrote:
> also "//build/win:default_exe_manifest" should be added to deps.

Ack, done. Thanks!

[Michael Achenbach, 2016-07-12 10:11:38]

gn lg

[rmcilroy, 2016-07-13 13:51:41]

I like it! Thanks for sticking with it, code looks easier to understand to me
and hopefully faster with the table. A couple of comments, but looks good once
bots are happy.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (left):

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:62: // eliminate last rather than
writing it.
No longer relevent?

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

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:53: void
BytecodePeepholeOptimizer::Write(BytecodeNode* node) { Optimize(node); }
Could we give this a new name, since it does the optimization and also writes
nodes to the next stage. Also, is there any way we could make WriteJump and
Write behave similarly with respect to sending things on to the next_stage
(i.e., WriteJump does it outside Optimize, but Write doesn't).

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:104: // The last bytecode can be
elided for the MAYBE cases if the last
What do we do with the MAYBE cases now? AFAIKT we don't do anything special here
any longer (so they are NO now), but we try to defer the emittion of the
expression positions until they are needed for throws in the
BytecodeArrayBuilder. If so, could you update this comment?

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:222: // Node can not be valid per
CanElideLastBasedOnSourcePosition().
Can we DCHECK this?

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:262: // Immediately emit current
node as nothing fuses with AddSmi, SubSmi, etc.
Could we not do this, we might want to fuse with AddSmi, etc. in the future.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:280: // Immediately emit current
node as nothing fuses with AddSmi, SubSmi, etc.
ditto

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:316: 
Should we be calling CanElideLastBasedOnSourcePosition here ?

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:21: bool FLAG_always_opt = false;
I don't think these should be necessary any longer after your preceeding CL.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:169: // zero immediate argument. This saves
dispatches, but not size.
second half of this comment is wrong (related to LdaZero I think).

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:357: DCHECK(RowsEqual(&table()->at(iter->second),
row));
Could we have this be part of the check instead of being a DCHECK (e.g.,:
if (iter != hash_to_row_map->end() && RowsEqual(&table()->at(iter->second),
row)) {
  ...

[oth, 2016-07-15 09:34:53]

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

[commit-bot: I haz the power, 2016-07-15 09:35:00]

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-07-15 09:52:31]

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

[commit-bot: I haz the power, 2016-07-15 09:52:32]

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

[oth, 2016-07-15 11:03:50]

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

[commit-bot: I haz the power, 2016-07-15 11:04:02]

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-07-15 11:27:11]

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

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

Dry run: This issue passed the CQ dry run.

[oth, 2016-07-15 13:03:51]

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

[commit-bot: I haz the power, 2016-07-15 13:03:57]

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

[oth, 2016-07-15 13:09:41]

Thanks for the comments. Revisions and comments attached.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (left):

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:62: // eliminate last rather than
writing it.
On 2016/07/13 13:51:40, rmcilroy wrote:
> No longer relevent?

CanElideLast() is gone.

There's no information about the surrounding code so the only safe elision here
would be when last_ is a NOP without source info. Added a new rule to avoid this
state and a check in SetLast that existing rules don't try to get there.

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

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:53: void
BytecodePeepholeOptimizer::Write(BytecodeNode* node) { Optimize(node); }
On 2016/07/13 13:51:41, rmcilroy wrote:
> Could we give this a new name, since it does the optimization and also writes
> nodes to the next stage. Also, is there any way we could make WriteJump and
> Write behave similarly with respect to sending things on to the next_stage
> (i.e., WriteJump does it outside Optimize, but Write doesn't).

Renamed ApplyPeepholeAction().

The answer to the rest is a little long.

Basically, per the comment in ApplyPeephole() having a single table gives the
best performance. Having different handler signatures and tables for applying
jump and non-jumps is slower.

For non-jump bytecodes the handlers run to completion based on the action.
Sometimes they emit the incoming node, sometimes not. Conditionally check this
after ApplyPeepholeAction slows performance.

WriteJump() always emits the incoming jump bytecode and has an additional label
argument. We don't really want this as part of the handler signature because
passing it again affects performance.

Happy to discuss, but we're in a local perf maxima. Added comments in Write()
and WriteJump().

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:104: // The last bytecode can be
elided for the MAYBE cases if the last
On 2016/07/13 13:51:41, rmcilroy wrote:
> What do we do with the MAYBE cases now? AFAIKT we don't do anything special
here
> any longer (so they are NO now), but we try to defer the emittion of the
> expression positions until they are needed for throws in the
> BytecodeArrayBuilder. If so, could you update this comment?

Done. Hopefully clearer now. We've not treated MAYBE differently from NO for a
good while. PTAL at the wording.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:222: // Node can not be valid per
CanElideLastBasedOnSourcePosition().
On 2016/07/13 13:51:41, rmcilroy wrote:
> Can we DCHECK this?

Hey! The code has just called CanElideLastBasedOnSourcePosition() and the
comment is referring to this. Shouldn't need to DCHECK condition still holds
here.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:262: // Immediately emit current
node as nothing fuses with AddSmi, SubSmi, etc.
On 2016/07/13 13:51:41, rmcilroy wrote:
> Could we not do this, we might want to fuse with AddSmi, etc. in the future.

How about adapting this when the future change is ready? Otherwise, there's dead
work doing the copy to last_ for something that might be needed. WDYT?

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:280: // Immediately emit current
node as nothing fuses with AddSmi, SubSmi, etc.
On 2016/07/13 13:51:41, rmcilroy wrote:
> ditto

Same here, propose only changing when needed.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:316: 
On 2016/07/13 13:51:41, rmcilroy wrote:
> Should we be calling CanElideLastBasedOnSourcePosition here ?

Added a DCHECK as there is an assumption that this always true in code that
reaches here.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:21: bool FLAG_always_opt = false;
On 2016/07/13 13:51:41, rmcilroy wrote:
> I don't think these should be necessary any longer after your preceeding CL.

Done. Thanks!

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:169: // zero immediate argument. This saves
dispatches, but not size.
On 2016/07/13 13:51:41, rmcilroy wrote:
> second half of this comment is wrong (related to LdaZero I think).

Done.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:357: DCHECK(RowsEqual(&table()->at(iter->second),
row));
On 2016/07/13 13:51:41, rmcilroy wrote:
> Could we have this be part of the check instead of being a DCHECK (e.g.,:
> if (iter != hash_to_row_map->end() && RowsEqual(&table()->at(iter->second),
> row)) {
>   ...

Added the comment above DCHECK to clarify as we discussed that changing the
check could allow de-duping to silently fail.

The hashing was more useful when this wasn't standalone. Considering removing
it.

[commit-bot: I haz the power, 2016-07-15 13:24:49]

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

[commit-bot: I haz the power, 2016-07-15 13:24:50]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-07-15 15:31:49]

lgtm once bots are happy and comments addressed.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (left):

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:62: // eliminate last rather than
writing it.
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:40, rmcilroy wrote:
> > No longer relevent?
> 
> CanElideLast() is gone.
> 
> There's no information about the surrounding code so the only safe elision
here
> would be when last_ is a NOP without source info. Added a new rule to avoid
this
> state and a check in SetLast that existing rules don't try to get there.

Great, thanks!

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

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:53: void
BytecodePeepholeOptimizer::Write(BytecodeNode* node) { Optimize(node); }
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > Could we give this a new name, since it does the optimization and also
writes
> > nodes to the next stage. Also, is there any way we could make WriteJump and
> > Write behave similarly with respect to sending things on to the next_stage
> > (i.e., WriteJump does it outside Optimize, but Write doesn't).
> 
> Renamed ApplyPeepholeAction().
> 
> The answer to the rest is a little long.
> 
> Basically, per the comment in ApplyPeephole() having a single table gives the
> best performance. Having different handler signatures and tables for applying
> jump and non-jumps is slower.
> 
> For non-jump bytecodes the handlers run to completion based on the action.
> Sometimes they emit the incoming node, sometimes not. Conditionally check this
> after ApplyPeepholeAction slows performance.
> 
> WriteJump() always emits the incoming jump bytecode and has an additional
label
> argument. We don't really want this as part of the handler signature because
> passing it again affects performance.
> 
> Happy to discuss, but we're in a local perf maxima. Added comments in Write()
> and WriteJump().

I see, right enough forgot about the label. Ok works for me, thanks for the
comment.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:104: // The last bytecode can be
elided for the MAYBE cases if the last
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > What do we do with the MAYBE cases now? AFAIKT we don't do anything special
> here
> > any longer (so they are NO now), but we try to defer the emittion of the
> > expression positions until they are needed for throws in the
> > BytecodeArrayBuilder. If so, could you update this comment?
> 
> Done. Hopefully clearer now. We've not treated MAYBE differently from NO for a
> good while. PTAL at the wording.

SGTM, thanks.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:222: // Node can not be valid per
CanElideLastBasedOnSourcePosition().
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > Can we DCHECK this?
> 
> Hey! The code has just called CanElideLastBasedOnSourcePosition() and the
> comment is referring to this. Shouldn't need to DCHECK condition still holds
> here.

Right, sorry I was confused since the comment sounded like the node is not valid
at this point (not the source position). Could the comment be clearer that it is
the source position that isn't valid?

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:262: // Immediately emit current
node as nothing fuses with AddSmi, SubSmi, etc.
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > Could we not do this, we might want to fuse with AddSmi, etc. in the future.
> 
> How about adapting this when the future change is ready? Otherwise, there's
dead
> work doing the copy to last_ for something that might be needed. WDYT?

I'd rather not do this since it would not be immediately apparent why these
bytecodes never get fused since this bit of code would not be near any future
change to cause AddSmi, etc. to be fused. We don't special case discarding of
last_ for other bytecodes that we currently never fuse, so I'd rather avoid this
special case if that's ok.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:21: bool FLAG_always_opt = false;
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > I don't think these should be necessary any longer after your preceeding CL.
> 
> Done. Thanks!

Still there?

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:169: // zero immediate argument. This saves
dispatches, but not size.
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > second half of this comment is wrong (related to LdaZero I think).
> 
> Done.

Still there?

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:357: DCHECK(RowsEqual(&table()->at(iter->second),
row));
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > Could we have this be part of the check instead of being a DCHECK (e.g.,:
> > if (iter != hash_to_row_map->end() && RowsEqual(&table()->at(iter->second),
> > row)) {
> >   ...
> 
> Added the comment above DCHECK to clarify as we discussed that changing the
> check could allow de-duping to silently fail.
> 
> The hashing was more useful when this wasn't standalone. Considering removing
> it.

Can't see the comment, did you forget to add the changes to file to the commit?

[oth, 2016-07-15 17:27:26]

Thanks. All done.

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

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:222: // Node can not be valid per
CanElideLastBasedOnSourcePosition().
On 2016/07/15 15:31:48, rmcilroy wrote:
> On 2016/07/15 13:09:40, oth wrote:
> > On 2016/07/13 13:51:41, rmcilroy wrote:
> > > Can we DCHECK this?
> > 
> > Hey! The code has just called CanElideLastBasedOnSourcePosition() and the
> > comment is referring to this. Shouldn't need to DCHECK condition still holds
> > here.
> 
> Right, sorry I was confused since the comment sounded like the node is not
valid
> at this point (not the source position). Could the comment be clearer that it
is
> the source position that isn't valid?

Definitely. Updated the comment.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:262: // Immediately emit current
node as nothing fuses with AddSmi, SubSmi, etc.
On 2016/07/15 15:31:48, rmcilroy wrote:
> On 2016/07/15 13:09:40, oth wrote:
> > On 2016/07/13 13:51:41, rmcilroy wrote:
> > > Could we not do this, we might want to fuse with AddSmi, etc. in the
future.
> > 
> > How about adapting this when the future change is ready? Otherwise, there's
> dead
> > work doing the copy to last_ for something that might be needed. WDYT?
> 
> I'd rather not do this since it would not be immediately apparent why these
> bytecodes never get fused since this bit of code would not be near any future
> change to cause AddSmi, etc. to be fused. We don't special case discarding of
> last_ for other bytecodes that we currently never fuse, so I'd rather avoid
this
> special case if that's ok. 

Done.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:280: // Immediately emit current
node as nothing fuses with AddSmi, SubSmi, etc.
On 2016/07/15 13:09:40, oth wrote:
> On 2016/07/13 13:51:41, rmcilroy wrote:
> > ditto
> 
> Same here, propose only changing when needed.

Done.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
File src/interpreter/mkpeephole.cc (right):

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:21: bool FLAG_always_opt = false;
On 2016/07/15 15:31:48, rmcilroy wrote:
> On 2016/07/15 13:09:40, oth wrote:
> > On 2016/07/13 13:51:41, rmcilroy wrote:
> > > I don't think these should be necessary any longer after your preceeding
CL.
> > 
> > Done. Thanks!
> 
> Still there?

Done...Again. Started removing hashing and decided to save it for later as not
wanting to introduce churn. Backed out the changes described as done here.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:169: // zero immediate argument. This saves
dispatches, but not size.
On 2016/07/15 15:31:48, rmcilroy wrote:
> On 2016/07/15 13:09:40, oth wrote:
> > On 2016/07/13 13:51:41, rmcilroy wrote:
> > > second half of this comment is wrong (related to LdaZero I think).
> > 
> > Done.
> 
> Still there?

Done.

https://codereview.chromium.org/2118183002/diff/600001/src/interpreter/mkpeep...
src/interpreter/mkpeephole.cc:357: DCHECK(RowsEqual(&table()->at(iter->second),
row));
On 2016/07/15 15:31:49, rmcilroy wrote:
> On 2016/07/15 13:09:40, oth wrote:
> > On 2016/07/13 13:51:41, rmcilroy wrote:
> > > Could we have this be part of the check instead of being a DCHECK (e.g.,:
> > > if (iter != hash_to_row_map->end() &&
RowsEqual(&table()->at(iter->second),
> > > row)) {
> > >   ...
> > 
> > Added the comment above DCHECK to clarify as we discussed that changing the
> > check could allow de-duping to silently fail.
> > 
> > The hashing was more useful when this wasn't standalone. Considering
removing
> > it.
> 
> Can't see the comment, did you forget to add the changes to file to the
commit?

Done.

[oth, 2016-07-18 07:16:53]

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

[commit-bot: I haz the power, 2016-07-18 07:17:01]

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

[oth, 2016-07-18 08:06:00]

The CQ bit was checked by oth@chromium.org

[oth, 2016-07-18 08:06:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2118183002/#ps730001
(title: "No change. Upload to run more CQ tests.")

[commit-bot: I haz the power, 2016-07-18 08:06:13]

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-07-18 08:32:50]

Committed patchset #38 (id:730001)

[commit-bot: I haz the power, 2016-07-18 08:35:06]

Description was changed from

==========
[interpeter] Move to table based peephole optimizer.

Introduces a lookup table for peephole optimizations.

Fixes some tests using BytecodePeepholeOptimizer::Write() that should
have been update to use BytecodePeepholeOptimizer::WriteJump().

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

to

==========
[interpeter] Move to table based peephole optimizer.

Introduces a lookup table for peephole optimizations.

Fixes some tests using BytecodePeepholeOptimizer::Write() that should
have been update to use BytecodePeepholeOptimizer::WriteJump().

BUG=v8:4280
LOG=N

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

[commit-bot: I haz the power, 2016-07-18 08:35:08]

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

[oth, 2016-07-18 08:51:31]

A revert of this CL (patchset #38 id:730001) has been created in
https://codereview.chromium.org/2161563002/ by oth@chromium.org.

The reason for reverting is: Break MIPS port..

[oth, 2016-07-18 15:20:25]

Description was changed from

==========
[interpeter] Move to table based peephole optimizer.

Introduces a lookup table for peephole optimizations.

Fixes some tests using BytecodePeepholeOptimizer::Write() that should
have been update to use BytecodePeepholeOptimizer::WriteJump().

BUG=v8:4280
LOG=N

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

to

==========
[interpeter] Move to table based peephole optimizer.

Introduces a lookup table for peephole optimizations.

Fixes some tests using BytecodePeepholeOptimizer::Write() that should
have been update to use BytecodePeepholeOptimizer::WriteJump().

BUG=v8:4280
LOG=N

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

[oth, 2016-07-18 16:31:45]

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

[commit-bot: I haz the power, 2016-07-18 16:32:07]

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-07-18 17:02:15]

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

[commit-bot: I haz the power, 2016-07-18 17:02:17]

Dry run: This issue passed the CQ dry run.

[oth, 2016-07-19 08:18:40]

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

[commit-bot: I haz the power, 2016-07-19 08:18:49]

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-07-19 08:53:16]

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

[commit-bot: I haz the power, 2016-07-19 08:53:17]

Dry run: This issue passed the CQ dry run.