[interpreter] Introduce fused bytecodes for common sequences.

[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into a destination register operand instead of the
accumulator. They are emitted by the peephole optimizer.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/25b3fe7961fce1dd39a21b0f717750a926eeb681
Cr-Commit-Position: refs/heads/master@{#36507}

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

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

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

Ross, PTAL, thanks.

[rmcilroy, 2016-05-17 15:49:34]

https://codereview.chromium.org/1985753002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1985753002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1440: Node* node = NewNode(op, context);
Could we factor out the common code to a BuildXXX() function which returns the
result node, and then just bind that either to the accumulator or register
depending upon Ldr / Lda. Also could you group the Lda and Ldr functions.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (left):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:111: const BytecodeNode* const
current) const {
Still const?

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:118: bool
BytecodePeepholeOptimizer::ChangeLdarMovToLdarStar(
This seems unrelated to the other optimizations. Can we land it in a different
CL?

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:130: void
BytecodePeepholeOptimizer::RewriteLastLoadAndStar(
The name confuses me. How about RewriteLdaToLdr.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:184: return
RemoveToBooleanFromJump(current) || ChangeLdaToLdr(current) ||
I think ChangeLdaToLdr should really be in CanElideCurrent - since in-effect it
is updating last to elide the current. I would prefer this (even if it means
renaming CanElideCurrent somehow) since it keeps the logic of updating the
source position when eliding the current instruction in one place. WDYT?

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:216: if
(!SimpleSubstitution(current)) {
Why the change in behaviour here? I think it makes sense to do
SimpleSubstitution then do CanElideCurrent irrespective of whether we
substituted it, in case there ends up being a elide optimization can happen even
after a substitution. WDYT?

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:268: OperandType::kIdx, OperandType::kRegOut)
I'd prefer these lived next to the Lda versions - we have other bytecodes which
are only emitted by the peephole optmizer and we don't put them in their own
group, I think it would be better to group by behaviour than how they are
emitted.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:404: void Interpreter::DoLdaGlobal(Callable ic,
InterpreterAssembler* assembler) {
I'd just remove these existing helpers - it's not worth it, we should just use
the new ones you are adding and do the SetAccumulator / Register and Dispatch in
the DoXXX functions.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:1802: // Loads undefined into the accumulator and
|reg|.
nit - also group these near Lda variants please

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter.h
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.h:136: // Dispatch().
I don't think it's necessary to mention the caller has to do dispatch. However,
please add a comment for each of these like the ones above.

[rmcilroy, 2016-05-18 16:09:42]

One more request:

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:134: V(StaKeyedPropertyStrict,
AccumulatorUse::kRead, OperandType::kReg,         \
Could we do this renaming in a seperate CL as well please? It is difficult to
eyeball the changes in the bytecode expectations coming from the Ldr fused
bytecodes with this mechanical change also in the same CL.

[oth, 2016-05-18 19:25:15]

Description was changed from

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into the accumulator and an additional destination register
operand. They are emitted by the peephole optimizer.

The change also updates the prefix on some existing bytecodes for
consistency. The Load prefix becomes Lda and Store become Sta.

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

to

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into the accumulator and an additional destination register
operand. They are emitted by the peephole optimizer.

The change also updates the prefix on some existing bytecodes for
consistency.

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

[oth, 2016-05-18 19:25:31]

Description was changed from

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into the accumulator and an additional destination register
operand. They are emitted by the peephole optimizer.

The change also updates the prefix on some existing bytecodes for
consistency.

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

to

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into the accumulator and an additional destination register
operand. They are emitted by the peephole optimizer.

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

[oth, 2016-05-18 19:25:47]

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

[commit-bot: I haz the power, 2016-05-18 19:25:54]

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

[commit-bot: I haz the power, 2016-05-18 19:56:37]

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

[commit-bot: I haz the power, 2016-05-18 19:56:37]

Dry run: This issue passed the CQ dry run.

[oth, 2016-05-18 20:22:25]

Thanks for the comments.

All incorporated except for the side from the suggestions around
CanElideCurrent.

If this lands, the existing naming inconsistencies can be nixed. Not going to
con-up a CL due to expected continuing churn with this.

https://codereview.chromium.org/1985753002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1985753002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1440: Node* node = NewNode(op, context);
On 2016/05/17 15:49:33, rmcilroy wrote:
> Could we factor out the common code to a BuildXXX() function which returns the
> result node, and then just bind that either to the accumulator or register
> depending upon Ldr / Lda. Also could you group the Lda and Ldr functions.

Done.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (left):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:111: const BytecodeNode* const
current) const {
On 2016/05/17 15:49:33, rmcilroy wrote:
> Still const?

Done. Oops, lost const when it incorporated the change subsequently proposed in
this review for CanElideCurrent().

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:118: bool
BytecodePeepholeOptimizer::ChangeLdarMovToLdarStar(
On 2016/05/17 15:49:33, rmcilroy wrote:
> This seems unrelated to the other optimizations. Can we land it in a different
> CL?

Done.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:130: void
BytecodePeepholeOptimizer::RewriteLastLoadAndStar(
On 2016/05/17 15:49:33, rmcilroy wrote:
> The name confuses me. How about RewriteLdaToLdr.

Changed to ChangeLoadStarToLdrLdar() which describes the transformation.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:184: return
RemoveToBooleanFromJump(current) || ChangeLdaToLdr(current) ||
On 2016/05/17 15:49:33, rmcilroy wrote:
> I think ChangeLdaToLdr should really be in CanElideCurrent - since in-effect
it
> is updating last to elide the current. I would prefer this (even if it means
> renaming CanElideCurrent somehow) since it keeps the logic of updating the
> source position when eliding the current instruction in one place. WDYT?

This was actually implemented during the construction of this CL and rejected.

CanElideCurrent() is a test for the elision of the incoming bytecode. The name
does not suggest producing side effects of re-writing bytecodes.

The SimpleSubstution is either replacing the current or replacing both the
current and last. In neither case is eliding the current node appropriate.

The Optimize method is pretty clear as is - one of three things is performed,
direct bytecode substitution, eliding the current, or eliding the last.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:216: if
(!SimpleSubstitution(current)) {
On 2016/05/17 15:49:33, rmcilroy wrote:
> Why the change in behaviour here? I think it makes sense to do
> SimpleSubstitution then do CanElideCurrent irrespective of whether we
> substituted it, in case there ends up being a elide optimization can happen
even
> after a substitution. WDYT?

As discussed above, this is not appealing. There is no elision in
SimpleSubstitution (clearly a mutating method) vs CanElideCurrent (a
mutation-effect free test).

If there is a possible optimization after the simple substitution then simple
substitution was not the appropriate path and it will be costly in terms of
performance to perform multiple rounds of rule evaluation. If there are cases in
future that actually need this, let's make the change. In the interim, YAGNI.

Pending perf, the table based approach makes this much easier.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:134: V(StaKeyedPropertyStrict,
AccumulatorUse::kRead, OperandType::kReg,         \
On 2016/05/18 16:09:41, rmcilroy wrote:
> Could we do this renaming in a seperate CL as well please? It is difficult to
> eyeball the changes in the bytecode expectations coming from the Ldr fused
> bytecodes with this mechanical change also in the same CL.

Done.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:268: OperandType::kIdx, OperandType::kRegOut)
On 2016/05/17 15:49:33, rmcilroy wrote:
> I'd prefer these lived next to the Lda versions - we have other bytecodes
which
> are only emitted by the peephole optmizer and we don't put them in their own
> group, I think it would be better to group by behaviour than how they are
> emitted.

Done.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:404: void Interpreter::DoLdaGlobal(Callable ic,
InterpreterAssembler* assembler) {
On 2016/05/17 15:49:33, rmcilroy wrote:
> I'd just remove these existing helpers - it's not worth it, we should just use
> the new ones you are adding and do the SetAccumulator / Register and Dispatch
in
> the DoXXX functions.

Done.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:1802: // Loads undefined into the accumulator and
|reg|.
On 2016/05/17 15:49:33, rmcilroy wrote:
> nit - also group these near Lda variants please

Done.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter.h
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.h:136: // Dispatch().
On 2016/05/17 15:49:33, rmcilroy wrote:
> I don't think it's necessary to mention the caller has to do dispatch.
However,
> please add a comment for each of these like the ones above.

Done.

[oth, 2016-05-18 20:23:10]

Description was changed from

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into the accumulator and an additional destination register
operand. They are emitted by the peephole optimizer.

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

to

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into al destination register
operand instead of the accumulator. They are emitted by the peephole optimizer.

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

[oth, 2016-05-18 20:23:40]

Description was changed from

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into al destination register
operand instead of the accumulator. They are emitted by the peephole optimizer.

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

to

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These load values
into a destination register operand instead of the accumulator. They are emitted
by the peephole optimizer.

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

[oth, 2016-05-18 20:24:12]

Description was changed from

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These load values
into a destination register operand instead of the accumulator. They are emitted
by the peephole optimizer.

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

to

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into a destination register operand instead of the
accumulator. They are emitted by the peephole optimizer.

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

[rmcilroy, 2016-05-19 10:01:02]

Pausing this review for now since you mention you prefer the table approach -
let me know if you want me to restart it.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:184: return
RemoveToBooleanFromJump(current) || ChangeLdaToLdr(current) ||
On 2016/05/18 20:22:25, oth wrote:
> On 2016/05/17 15:49:33, rmcilroy wrote:
> > I think ChangeLdaToLdr should really be in CanElideCurrent - since in-effect
> it
> > is updating last to elide the current. I would prefer this (even if it means
> > renaming CanElideCurrent somehow) since it keeps the logic of updating the
> > source position when eliding the current instruction in one place. WDYT?
> 
> This was actually implemented during the construction of this CL and rejected.
> 
> CanElideCurrent() is a test for the elision of the incoming bytecode. The name
> does not suggest producing side effects of re-writing bytecodes.
> 
> The SimpleSubstution is either replacing the current or replacing both the
> current and last. In neither case is eliding the current node appropriate.
> 
> The Optimize method is pretty clear as is - one of three things is performed,
> direct bytecode substitution, eliding the current, or eliding the last.

Acknowledged.

https://codereview.chromium.org/1985753002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:216: if
(!SimpleSubstitution(current)) {
On 2016/05/18 20:22:25, oth wrote:
> On 2016/05/17 15:49:33, rmcilroy wrote:
> > Why the change in behaviour here? I think it makes sense to do
> > SimpleSubstitution then do CanElideCurrent irrespective of whether we
> > substituted it, in case there ends up being a elide optimization can happen
> even
> > after a substitution. WDYT?
> 
> As discussed above, this is not appealing. There is no elision in
> SimpleSubstitution (clearly a mutating method) vs CanElideCurrent (a
> mutation-effect free test).
> 
> If there is a possible optimization after the simple substitution then simple
> substitution was not the appropriate path and it will be costly in terms of
> performance to perform multiple rounds of rule evaluation. If there are cases
in
> future that actually need this, let's make the change. In the interim, YAGNI.
> 
> Pending perf, the table based approach makes this much easier.

Acknowledged.

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

https://codereview.chromium.org/1985753002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:186:
RemoveToBooleanFromLogicalNot(current);
nit - RemoveTo calls together.

https://codereview.chromium.org/1985753002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:219: if
(current->source_info().is_valid()) {
As discussed offline, I think this should be if
(current->source_info().is_valid() && current->source_info().is_statement()) {

[oth, 2016-05-19 10:32:03]

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

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

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

[oth, 2016-05-19 10:35:23]

Breaking the table approach into multiple pieces is going to be a tonne of work
and we're trying to move on. It already targets this point. Comments addressed
if you want to continue.

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

https://codereview.chromium.org/1985753002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:186:
RemoveToBooleanFromLogicalNot(current);
On 2016/05/19 10:01:02, rmcilroy wrote:
> nit - RemoveTo calls together.

Done.

https://codereview.chromium.org/1985753002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:219: if
(current->source_info().is_valid()) {
On 2016/05/19 10:01:02, rmcilroy wrote:
> As discussed offline, I think this should be if
> (current->source_info().is_valid() && current->source_info().is_statement()) {
> 

The is_statement() was a daft mistake. Gone.

[commit-bot: I haz the power, 2016-05-19 11:05:02]

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

[commit-bot: I haz the power, 2016-05-19 11:05:04]

Dry run: This issue passed the CQ dry run.

[oth, 2016-05-23 10:11:28]

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

[commit-bot: I haz the power, 2016-05-23 10:11:35]

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

[oth, 2016-05-23 10:22:37]

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

[oth, 2016-05-23 10:22:38]

mstarzinger@chromium.org: Please review any and all changes here, but
particularly the BytecodeGraphBuilder changes.

rmcilroy@chromium.org: the table based peephole optimizer is set aside for the
time being pending comprehension of perf differences.

Thanks
Orion

[commit-bot: I haz the power, 2016-05-23 10:42:47]

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

[commit-bot: I haz the power, 2016-05-23 10:42:48]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-05-24 10:53:57]

Looks great, just nits other than the typo in bytecode-graph-builder.cc.

LGTM once they are addressed.

https://codereview.chromium.org/1985753002/diff/100001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1985753002/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:652: void
BytecodeGraphBuilder::VisitLdrGlobal() {
nit - move below VisitLdaGlobal

https://codereview.chromium.org/1985753002/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:654: Node* node =
BuildLoadGlobal(TypeofMode::INSIDE_TYPEOF);
This should be NOT_INSIDE_OF_TYPEOF.

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

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:97: void
BytecodePeepholeOptimizer::TryToRemoveLastExpressionPosition(
nit MaybeRemoveLastExpressionPosition

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:171:
last_.source_info().Update(current->source_info());
nit - comment on why you move the source position backwards.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:230: bool
BytecodePeepholeOptimizer::UpdateLastAndCurrentBytecodes(
nit - s/UpdateLastAndCurrentBytecodes/CanTransformLastAndCurrent/ (or
CanChangeLastAndCurrent)

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:265: if
(!UpdateLastAndCurrentBytecodes(current)) {
nit - rather than if nesting, how about early returns, i.e.:

if (CanTransformLastAndCurrent(current)) {
  return current;
} 

if (CanElideCurrent(current)) {
  if (current->source_info().is_valid()) {
    current->set_bytecode(Bytecode::kNop);
  } else {
    current = nullptr;
  }
  return current;
}

if (CanElideLast(current) &&
    CanElideLastBasedOnSourcePosition(current)) {
  current->source_info().Update(last_.source_info());
  InvalidateLast();
  return current;
} 

return current;

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

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:40: void
ChangeLoadStarToLdrLdar(Bytecode new_bytecode,
This is a helper function for ChangeLoadToLdr, not a function called by
SimpleSubstitution directly, and I think the name is a bit confusingly similar
to ChangeLoadtoLdr as well. How about making this a static (annoymous name
space) helper which you pass last_ to explicitly and calling it
TransformLdaToLdr.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:44: bool
ChangeLoadToLdr(BytecodeNode* const current);
nit - ChangeLdaToLdr ?

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (left):

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:295: 
Thanks for the cleanup!

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:299: Node*
Interpreter::BuildLoadUndefined(InterpreterAssembler* assembler) {
nit - I think we could just inline this

https://codereview.chromium.org/1985753002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right):

https://codereview.chromium.org/1985753002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/bytecode_expectations/ForOf.golden:277: B(Ldar), R(5),
Interesting situation where we depend on the Ldar :)

https://codereview.chromium.org/1985753002/diff/100001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-peephole-optimizer-unittest.cc (right):

https://codereview.chromium.org/1985753002/diff/100001/test/unittests/interpr...
test/unittests/interpreter/bytecode-peephole-optimizer-unittest.cc:383: // Tests
covering BytecodePeepholeOptimizer::RewriteLastAndCurrent().
nit - update comment with the name you choose for the function.

[oth, 2016-05-24 13:37:12]

Thanks, very useful. The snafu with the typeof could have been ugly.

All incorporate with mild push back on a couple of naming nits. If you feel
strongly, after reading the pushback then let's find some middle ground.

https://codereview.chromium.org/1985753002/diff/100001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1985753002/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:652: void
BytecodeGraphBuilder::VisitLdrGlobal() {
On 2016/05/24 10:53:56, rmcilroy wrote:
> nit - move below VisitLdaGlobal

Done.

https://codereview.chromium.org/1985753002/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:654: Node* node =
BuildLoadGlobal(TypeofMode::INSIDE_TYPEOF);
On 2016/05/24 10:53:56, rmcilroy wrote:
> This should be NOT_INSIDE_OF_TYPEOF.

Done. Ouch, thanks!

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

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:97: void
BytecodePeepholeOptimizer::TryToRemoveLastExpressionPosition(
On 2016/05/24 10:53:56, rmcilroy wrote:
> nit MaybeRemoveLastExpressionPosition

IMO The prefix TryTo better fits what the function does than Maybe. Maybe
suggests a degree of arbitrariness on whether it occurs.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:171:
last_.source_info().Update(current->source_info());
On 2016/05/24 10:53:56, rmcilroy wrote:
> nit - comment on why you move the source position backwards.

Done.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:230: bool
BytecodePeepholeOptimizer::UpdateLastAndCurrentBytecodes(
On 2016/05/24 10:53:56, rmcilroy wrote:
> nit - s/UpdateLastAndCurrentBytecodes/CanTransformLastAndCurrent/ (or
> CanChangeLastAndCurrent)

The prefix 'Can' suggests testing a property of the pair, not actually
performing the update. The point of use will look confusing as there will be no
corresponding action for CanTransformLastAndCurrent() wherease the other Can
prefixed methods have associated actions in Optimize().

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:265: if
(!UpdateLastAndCurrentBytecodes(current)) {
On 2016/05/24 10:53:56, rmcilroy wrote:
> nit - rather than if nesting, how about early returns, i.e.:
> 
> if (CanTransformLastAndCurrent(current)) {
>   return current;
> } 
> 
> if (CanElideCurrent(current)) {
>   if (current->source_info().is_valid()) {
>     current->set_bytecode(Bytecode::kNop);
>   } else {
>     current = nullptr;
>   }
>   return current;
> }
> 
> if (CanElideLast(current) &&
>     CanElideLastBasedOnSourcePosition(current)) {
>   current->source_info().Update(last_.source_info());
>   InvalidateLast();
>   return current;
> } 
> 
> return current;

Done generally have a mild leaning towards single return (or guarded return) for
readability and maintenance risk. Maybe just an age thing :-)

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

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:40: void
ChangeLoadStarToLdrLdar(Bytecode new_bytecode,
On 2016/05/24 10:53:56, rmcilroy wrote:
> This is a helper function for ChangeLoadToLdr, not a function called by
> SimpleSubstitution directly, and I think the name is a bit confusingly similar
> to ChangeLoadtoLdr as well. How about making this a static (annoymous name
> space) helper which you pass last_ to explicitly and calling it
> TransformLdaToLdr.

Done modulo the name of the internal function keeps the Star and Ldar components
to convey the pairwise transformation happening here.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.h:44: bool
ChangeLoadToLdr(BytecodeNode* const current);
On 2016/05/24 10:53:56, rmcilroy wrote:
> nit - ChangeLdaToLdr ?

Done.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (left):

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:295: 
On 2016/05/24 10:53:56, rmcilroy wrote:
> Thanks for the cleanup!

Acknowledged.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:299: Node*
Interpreter::BuildLoadUndefined(InterpreterAssembler* assembler) {
On 2016/05/24 10:53:56, rmcilroy wrote:
> nit - I think we could just inline this

Done.

https://codereview.chromium.org/1985753002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right):

https://codereview.chromium.org/1985753002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/bytecode_expectations/ForOf.golden:277: B(Ldar), R(5),
On 2016/05/24 10:53:56, rmcilroy wrote:
> Interesting situation where we depend on the Ldar :)

Yes. It's a funny function too with 2 lines of javascript producing ~360 bytes
of bytecode.

https://codereview.chromium.org/1985753002/diff/100001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-peephole-optimizer-unittest.cc (right):

https://codereview.chromium.org/1985753002/diff/100001/test/unittests/interpr...
test/unittests/interpreter/bytecode-peephole-optimizer-unittest.cc:383: // Tests
covering BytecodePeepholeOptimizer::RewriteLastAndCurrent().
On 2016/05/24 10:53:57, rmcilroy wrote:
> nit - update comment with the name you choose for the function.

Done.

[rmcilroy, 2016-05-24 14:00:11]

LGTM.

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

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:97: void
BytecodePeepholeOptimizer::TryToRemoveLastExpressionPosition(
On 2016/05/24 13:37:12, oth wrote:
> On 2016/05/24 10:53:56, rmcilroy wrote:
> > nit MaybeRemoveLastExpressionPosition
> 
> IMO The prefix TryTo better fits what the function does than Maybe. Maybe
> suggests a degree of arbitrariness on whether it occurs.

Acknowledged.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:230: bool
BytecodePeepholeOptimizer::UpdateLastAndCurrentBytecodes(
On 2016/05/24 13:37:12, oth wrote:
> On 2016/05/24 10:53:56, rmcilroy wrote:
> > nit - s/UpdateLastAndCurrentBytecodes/CanTransformLastAndCurrent/ (or
> > CanChangeLastAndCurrent)
> 
> The prefix 'Can' suggests testing a property of the pair, not actually
> performing the update. The point of use will look confusing as there will be
no
> corresponding action for CanTransformLastAndCurrent() wherease the other Can
> prefixed methods have associated actions in Optimize().

OK, agree on "Can". I would still prefer Update is renamed to Transform or
Change though.

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:265: if
(!UpdateLastAndCurrentBytecodes(current)) {
On 2016/05/24 13:37:12, oth wrote:
> On 2016/05/24 10:53:56, rmcilroy wrote:
> > nit - rather than if nesting, how about early returns, i.e.:
> > 
> > if (CanTransformLastAndCurrent(current)) {
> >   return current;
> > } 
> > 
> > if (CanElideCurrent(current)) {
> >   if (current->source_info().is_valid()) {
> >     current->set_bytecode(Bytecode::kNop);
> >   } else {
> >     current = nullptr;
> >   }
> >   return current;
> > }
> > 
> > if (CanElideLast(current) &&
> >     CanElideLastBasedOnSourcePosition(current)) {
> >   current->source_info().Update(last_.source_info());
> >   InvalidateLast();
> >   return current;
> > } 
> > 
> > return current;
> 
> Done generally have a mild leaning towards single return (or guarded return)
for
> readability and maintenance risk. Maybe just an age thing :-)

I agree in the general case, however I think this is cleaner to avoid the deeper
nesting. Thanks.

https://codereview.chromium.org/1985753002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right):

https://codereview.chromium.org/1985753002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/bytecode_expectations/ForOf.golden:277: B(Ldar), R(5),
On 2016/05/24 13:37:12, oth wrote:
> On 2016/05/24 10:53:56, rmcilroy wrote:
> > Interesting situation where we depend on the Ldar :)
> 
> Yes. It's a funny function too with 2 lines of javascript producing ~360 bytes
> of bytecode.

That's ForOf for you ;).

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

https://codereview.chromium.org/1985753002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:171: namespace {
nit - newline after {

https://codereview.chromium.org/1985753002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:197: }  // namespace
nit - newline above

https://codereview.chromium.org/1985753002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:305: InvalidateLast();
Please add "return current" here too ensure we don't fall through accidentally
to  another condition if we add one later.

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

Done. Thanks!

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

https://codereview.chromium.org/1985753002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:230: bool
BytecodePeepholeOptimizer::UpdateLastAndCurrentBytecodes(
On 2016/05/24 14:00:10, rmcilroy wrote:
> On 2016/05/24 13:37:12, oth wrote:
> > On 2016/05/24 10:53:56, rmcilroy wrote:
> > > nit - s/UpdateLastAndCurrentBytecodes/CanTransformLastAndCurrent/ (or
> > > CanChangeLastAndCurrent)
> > 
> > The prefix 'Can' suggests testing a property of the pair, not actually
> > performing the update. The point of use will look confusing as there will be
> no
> > corresponding action for CanTransformLastAndCurrent() wherease the other Can
> > prefixed methods have associated actions in Optimize().
> 
> OK, agree on "Can". I would still prefer Update is renamed to Transform or
> Change though.

Done.

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

https://codereview.chromium.org/1985753002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:171: namespace {
On 2016/05/24 14:00:11, rmcilroy wrote:
> nit - newline after {

Done.

https://codereview.chromium.org/1985753002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:197: }  // namespace
On 2016/05/24 14:00:11, rmcilroy wrote:
> nit - newline above

Done.

https://codereview.chromium.org/1985753002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:305: InvalidateLast();
On 2016/05/24 14:00:11, rmcilroy wrote:
> Please add "return current" here too ensure we don't fall through accidentally
> to  another condition if we add one later.

Done.

[oth, 2016-05-24 15:11:33]

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

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

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

[commit-bot: I haz the power, 2016-05-24 15:51:48]

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

[commit-bot: I haz the power, 2016-05-24 15:51:49]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-05-25 08:47:36]

LGTM on "compiler".

[oth, 2016-05-25 09:17:51]

Thanks for the reviews. Chocks away!

[oth, 2016-05-25 09:23:20]

The CQ bit was checked by oth@chromium.org

[oth, 2016-05-25 09:23:21]

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

[commit-bot: I haz the power, 2016-05-25 09:23:29]

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

[commit-bot: I haz the power, 2016-05-25 09:55:30]

Description was changed from

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into a destination register operand instead of the
accumulator. They are emitted by the peephole optimizer.

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

to

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into a destination register operand instead of the
accumulator. They are emitted by the peephole optimizer.

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

[commit-bot: I haz the power, 2016-05-25 09:55:32]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-25 09:56:57]

Description was changed from

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into a destination register operand instead of the
accumulator. They are emitted by the peephole optimizer.

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

to

==========
[interpreter] Introduce fused bytecodes for common sequences.

This change introduces five fused bytecodes for common bytecode
sequences on popular websites. These are LdrNamedProperty,
LdrKeyedProperty, LdrGlobal, LdrContextSlot, and LdrUndefined. These
load values into a destination register operand instead of the
accumulator. They are emitted by the peephole optimizer.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/25b3fe7961fce1dd39a21b0f717750a926eeb681
Cr-Commit-Position: refs/heads/master@{#36507}
==========

[commit-bot: I haz the power, 2016-05-25 09:56:59]

Patchset 12 (id:??) landed as
https://crrev.com/25b3fe7961fce1dd39a21b0f717750a926eeb681
Cr-Commit-Position: refs/heads/master@{#36507}