[Interpreter] Ensure ValueOf is only called once for post-increment operations.

[Interpreter] Ensure ValueOf is only called once for post-increment operations.

BUG=chromium:662418
Committed: https://crrev.com/ba5885cc8b99951f445096349dd6a7a5a76e354b
Cr-Commit-Position: refs/heads/master@{#40846}

[rmcilroy, 2016-11-04 16:29:01]

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

[commit-bot: I haz the power, 2016-11-04 16:29:08]

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

[rmcilroy, 2016-11-04 16:29:15]

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

[rmcilroy, 2016-11-04 16:29:16]

Michi, PTAL thanks.

[commit-bot: I haz the power, 2016-11-04 16:42:38]

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

[commit-bot: I haz the power, 2016-11-04 16:42:39]

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

[rmcilroy, 2016-11-04 20:16:20]

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

[commit-bot: I haz the power, 2016-11-04 20:16:24]

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-11-04 20:53:44]

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

[commit-bot: I haz the power, 2016-11-04 20:53:44]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-11-07 16:18:01]

Ping?

[rmcilroy, 2016-11-08 14:29:56]

rmcilroy@chromium.org changed reviewers:
+ mythria@chromium.org
- mstarzinger@chromium.org

[rmcilroy, 2016-11-08 14:29:58]

Mythri, could you take a look please?

[Jarin, 2016-11-08 14:39:28]

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

[Jarin, 2016-11-08 14:39:29]

https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2755:
.LoadAccumulatorWithRegister(old_value);
Does not this mean that the feedback for the binop will be a bit bogus?

[mythria, 2016-11-08 14:58:58]

lgtm.

[rmcilroy, 2016-11-08 15:00:30]

On 2016/11/08 14:39:29, Jarin wrote:
>
https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecode-generator.cc:2755:
> .LoadAccumulatorWithRegister(old_value);
> Does not this mean that the feedback for the binop will be a bit bogus?

I'm not sure what you mean by this? It would probably have been bogus before
this change since we would be collecting feedback on the Inc/Dec with the
original (possibly non-number) operation, when we should have been toNumbering
it beforehand. This might have impacted some perf on TF - did you see this
anywhere?

[Jarin, 2016-11-08 15:24:52]

On 2016/11/08 15:00:30, rmcilroy wrote:
> On 2016/11/08 14:39:29, Jarin wrote:
> >
>
https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
> > File src/interpreter/bytecode-generator.cc (right):
> > 
> >
>
https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
> > src/interpreter/bytecode-generator.cc:2755:
> > .LoadAccumulatorWithRegister(old_value);
> > Does not this mean that the feedback for the binop will be a bit bogus?
> 
> I'm not sure what you mean by this? It would probably have been bogus before
> this change since we would be collecting feedback on the Inc/Dec with the
> original (possibly non-number) operation, when we should have been toNumbering
> it beforehand. This might have impacted some perf on TF - did you see this
> anywhere?

I think (I might be wrong) that for postfix we will not be really able 
to use feedback for the count operation because there will be the ToNumber
operation 
and we do not collect feedback for ToNumber. So even if we are always getting
Smi
into the ToNumber, we will not figure out that we can just do smi check and 
integer add. Instead we will have to call ToNumber from TF. No?

Note: I believe crankshaft optimistically assumes that you only apply count
operation to numbers, and deopts if you pass in anything else. We were 
hoping to avoid this kind of deopt loop in TF.

Anyways, this is for longer discussion, thanks for the clarifications. lgtm.

[rmcilroy, 2016-11-08 16:11:30]

On 2016/11/08 15:24:52, Jarin wrote:
> On 2016/11/08 15:00:30, rmcilroy wrote:
> > On 2016/11/08 14:39:29, Jarin wrote:
> > >
> >
>
https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
> > > File src/interpreter/bytecode-generator.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2473223004/diff/20001/src/interpreter/bytecod...
> > > src/interpreter/bytecode-generator.cc:2755:
> > > .LoadAccumulatorWithRegister(old_value);
> > > Does not this mean that the feedback for the binop will be a bit bogus?
> > 
> > I'm not sure what you mean by this? It would probably have been bogus before
> > this change since we would be collecting feedback on the Inc/Dec with the
> > original (possibly non-number) operation, when we should have been
toNumbering
> > it beforehand. This might have impacted some perf on TF - did you see this
> > anywhere?
> 
> I think (I might be wrong) that for postfix we will not be really able 
> to use feedback for the count operation because there will be the ToNumber
> operation 
> and we do not collect feedback for ToNumber. So even if we are always getting
> Smi
> into the ToNumber, we will not figure out that we can just do smi check and 
> integer add. Instead we will have to call ToNumber from TF. No?

Yeah this sounds right enough, however this should have been an issue in TF for
the AST graph builder too (since there is no type feedback on ToNumber anyway).
Can you analyse the type feedback for the input to the ToNumber - if that's
guaranteed to be a SMI then the ToNumber should be a noop. I guess this is for
the situations when you don't have type feedback on the input?

> Note: I believe crankshaft optimistically assumes that you only apply count
> operation to numbers, and deopts if you pass in anything else. We were 
> hoping to avoid this kind of deopt loop in TF.
> 
> Anyways, this is for longer discussion, thanks for the clarifications. lgtm.

Yeah, we could do that too but as you say this would be a deopt loop. The only
other option I can think of is to collect feedback on ToNumber, but this might
be overkill if we can't find anywhere that it matters.

[rmcilroy, 2016-11-08 16:35:37]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2016-11-08 16:35:44]

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-11-08 17:02:54]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
[Interpreter] Ensure ValueOf is only called once for post-increment operations.

BUG=chromium:662418
==========

to

==========
[Interpreter] Ensure ValueOf is only called once for post-increment operations.

BUG=chromium:662418

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

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

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