[Interpreter] Collect type feedback for subtract operation and pass it to turbofan.

[Interpreter] Collect type feedback for subtract operation and pass it to turbofan.

Collect type feedback for subtract operation in interpreter. Also use it in
bytecode-graph-bulder to set the correct Hint for subtract operation.

BUG=v8:5273
LOG=N

Committed: https://crrev.com/26e2d16b8f1efef81790ca246f50c9fa24971f3b
Cr-Commit-Position: refs/heads/master@{#38467}

[mythria, 2016-08-08 04:15:50]

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

[commit-bot: I haz the power, 2016-08-08 04:16: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-08-08 04:32:11]

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

[commit-bot: I haz the power, 2016-08-08 04:32:12]

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...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10473)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[mythria, 2016-08-08 04:33:55]

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

[mythria, 2016-08-08 04:33:56]

Hi Benedikt and Ross,

 I am sorry I could not finish binary op feedback before my vacation. There are
still a couple of things missing in this cl:

1. Initialize the type feedback slot to uninitialized state (Smi(0)).
2. Verify it behaves as intended. I just checked it does not break anything, I
am yet to verify it is actually passing the correct feedback to turbofan.

I have also added few comments in the code. I will be very happy to fix any
comments/suggestions from you and get it more closer to landing before I hand it
over to someone else in turbofan team.  Please let me know your comments. 

Thanks,
Mythri

https://codereview.chromium.org/2221833002/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2221833002/diff/1/src/code-stubs.h#newcode445
src/code-stubs.h:445: void GenerateAssembly(CodeStubAssembler* assembler) const
override {         \
I am not sure where GenerateAssembly is used. I did not add two additional
parameters because we collect type feedback only from ignition and others don't
require to collect type feedback.

https://codereview.chromium.org/2221833002/diff/1/src/code-stubs.h#newcode766
src/code-stubs.h:766:
DEFINE_TURBOFAN_BINARY_OP_CODE_STUB_WITH_FEEDBACK(Subtract, TurboFanCodeStub);
We can remove this once we migrate all binary ops to collect type feedback.

https://codereview.chromium.org/2221833002/diff/1/src/compiler/type-hint-anal...
File src/compiler/type-hint-analyzer.h (right):

https://codereview.chromium.org/2221833002/diff/1/src/compiler/type-hint-anal...
src/compiler/type-hint-analyzer.h:53: BinaryOperationHints::Hint
BinaryOperationHintFromFeedback(int type_feedback);
I am not sure this is the right place. Please let me know if there is a better
place to include this function.

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h
File src/ic/ic-state.h (right):

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h#newcode263
src/ic/ic-state.h:263: };
I am not sure where is the right place to add this. This is not exactly
ic-state, so this is not the right place I think.

[Benedikt Meurer, 2016-08-08 04:56:14]

Thanks for sharing this code.

https://codereview.chromium.org/2221833002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2221833002/diff/1/src/code-stubs.cc#newcode926
src/code-stubs.cc:926: Variable var_type_feedback(assembler,
MachineRepresentation::kWord32);
I'm not sure if this will scale, because

(a) it'll affect the code generated for the non feedback case, as we cannot get
rid of the dead feedback collection code, and
(b) makes these already tricky stubs a bit harder to read.

I'm tempted to suggest that we just make copies of the code for feedback; also
since we only need the fast case for feedback anyways (i.e. both sides at least
number), we could also only inline that into the bytecode handler and call out
to the stub for everything else (setting the feedback to any before).

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h
File src/ic/ic-state.h (right):

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h#newcode257
src/ic/ic-state.h:257: // code-stub and bytecode-graph-builder.
I'd rather use a very simple enum to encode the lattice, i.e. similar to what
hydrogen-types.h did. Something along the lines of:

class BinaryOperationFeedback {
 public:
  enum {
    kNone = 0x00,
    kSignedSmall = 0x01,
    kNumber = 0x3,
    kAny = 0x7
  };
};

Or some enum class even.

That way it's easier to just use the enum names in the code and you still
preserve the simple ORing for combining.

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h#newcode263
src/ic/ic-state.h:263: };
Please move it to globals.h for now.

[mythria, 2016-08-08 07:09:17]

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

[commit-bot: I haz the power, 2016-08-08 07:09:23]

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

[mythria, 2016-08-08 07:17:36]

Thanks Benedikt. I addressed your comments. I also added a check to see if the
feedback is uninitialized before combining it. I tried initializing the feedback
to Smi::FromInt(0) in TypeFeedbackVector::New to avoid this check, but there
were some problems. If that is not expected, I can have a look at that during
evening. 

Thanks,
Mythri

https://codereview.chromium.org/2221833002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2221833002/diff/1/src/code-stubs.cc#newcode926
src/code-stubs.cc:926: Variable var_type_feedback(assembler,
MachineRepresentation::kWord32);
On 2016/08/08 04:56:13, Benedikt Meurer wrote:
> I'm not sure if this will scale, because
> 
> (a) it'll affect the code generated for the non feedback case, as we cannot
get
> rid of the dead feedback collection code, and
> (b) makes these already tricky stubs a bit harder to read.
> 
> I'm tempted to suggest that we just make copies of the code for feedback; also
> since we only need the fast case for feedback anyways (i.e. both sides at
least
> number), we could also only inline that into the bytecode handler and call out
> to the stub for everything else (setting the feedback to any before).

Thanks Benedikt, I add a subtract stub that deals with Numbers that collects
type feedback. I did not try optimizing so we can directly jump to ToNumber
case. Done.

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h
File src/ic/ic-state.h (right):

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h#newcode257
src/ic/ic-state.h:257: // code-stub and bytecode-graph-builder.
On 2016/08/08 04:56:14, Benedikt Meurer wrote:
> I'd rather use a very simple enum to encode the lattice, i.e. similar to what
> hydrogen-types.h did. Something along the lines of:
> 
> class BinaryOperationFeedback {
>  public:
>   enum {
>     kNone = 0x00,
>     kSignedSmall = 0x01,
>     kNumber = 0x3,
>     kAny = 0x7
>   };
> };
> 
> Or some enum class even.
> 
> That way it's easier to just use the enum names in the code and you still
> preserve the simple ORing for combining. 

Thanks for the detailed comment Benedikt. Done.

https://codereview.chromium.org/2221833002/diff/1/src/ic/ic-state.h#newcode263
src/ic/ic-state.h:263: };
On 2016/08/08 04:56:13, Benedikt Meurer wrote:
> Please move it to globals.h for now.

Done.

[commit-bot: I haz the power, 2016-08-08 07:33:42]

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

[commit-bot: I haz the power, 2016-08-08 07:33:43]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-08-08 08:07:46]

Thanks, LGTM.

[rmcilroy, 2016-08-08 08:27:32]

Lgtm, thanks for getting this uploaded!

https://codereview.chromium.org/2221833002/diff/20001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2221833002/diff/20001/src/code-stubs.cc#newco...
src/code-stubs.cc:1254: assembler->Bind(&end);
Could we rename this variable since its no longer the end?

https://codereview.chromium.org/2221833002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2221833002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1145: // record type feedback.
Could you just do this now so that it's reused when we add more?

https://codereview.chromium.org/2221833002/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/2221833002/diff/20001/src/globals.h#newcode1155
src/globals.h:1155: enum { kNone = 0x00, kSignedSmall = 0x01, kNumber = 0x3,
kAny = 0x7 };
Nit - comment that the values are important to allow combining by binary ORing.

[rmcilroy, 2016-08-08 10:03:20]

Description was changed from

==========
[Interpreter] Collect type feedback for subtract operation and pass it to
turbofan.

Collect type feedback for subtract operation in interpreter. Also use it in
bytecode-graph-bulder to set the correct Hint for subtract operation.

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

to

==========
[Interpreter] Collect type feedback for subtract operation and pass it to
turbofan.

Collect type feedback for subtract operation in interpreter. Also use it in
bytecode-graph-bulder to set the correct Hint for subtract operation.

BUG=v8:5273
LOG=N
==========

[rmcilroy, 2016-08-08 10:19:01]

I updated the description to point to issue 5273 (a new tracking bug for binary
op type feedback).

[mythria, 2016-08-09 00:51:59]

Thanks Ross, fixed them.
Thank you both for the quick reviews.

https://codereview.chromium.org/2221833002/diff/20001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2221833002/diff/20001/src/code-stubs.cc#newco...
src/code-stubs.cc:1254: assembler->Bind(&end);
On 2016/08/08 08:27:31, rmcilroy wrote:
> Could we rename this variable since its no longer the end? 

Done.

https://codereview.chromium.org/2221833002/diff/20001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2221833002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1145: // record type feedback.
On 2016/08/08 08:27:31, rmcilroy wrote:
> Could you just do this now so that it's reused when we add more?

Done.

https://codereview.chromium.org/2221833002/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/2221833002/diff/20001/src/globals.h#newcode1155
src/globals.h:1155: enum { kNone = 0x00, kSignedSmall = 0x01, kNumber = 0x3,
kAny = 0x7 };
On 2016/08/08 08:27:31, rmcilroy wrote:
> Nit - comment that the values are important to allow combining by binary
ORing.

Done.

[mythria, 2016-08-09 00:52:08]

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

[commit-bot: I haz the power, 2016-08-09 00:52:17]

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-08-09 01:17:01]

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

[commit-bot: I haz the power, 2016-08-09 01:17:02]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-08-09 01:24:11]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-08-09 01:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2221833002/#ps40001
(title: "Addressed comments from Ross.")

[commit-bot: I haz the power, 2016-08-09 01:24:21]

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-08-09 06:48:06]

Description was changed from

==========
[Interpreter] Collect type feedback for subtract operation and pass it to
turbofan.

Collect type feedback for subtract operation in interpreter. Also use it in
bytecode-graph-bulder to set the correct Hint for subtract operation.

BUG=v8:5273
LOG=N
==========

to

==========
[Interpreter] Collect type feedback for subtract operation and pass it to
turbofan.

Collect type feedback for subtract operation in interpreter. Also use it in
bytecode-graph-bulder to set the correct Hint for subtract operation.

BUG=v8:5273
LOG=N
==========

[commit-bot: I haz the power, 2016-08-09 06:48:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-09 06:49:32]

Description was changed from

==========
[Interpreter] Collect type feedback for subtract operation and pass it to
turbofan.

Collect type feedback for subtract operation in interpreter. Also use it in
bytecode-graph-bulder to set the correct Hint for subtract operation.

BUG=v8:5273
LOG=N
==========

to

==========
[Interpreter] Collect type feedback for subtract operation and pass it to
turbofan.

Collect type feedback for subtract operation in interpreter. Also use it in
bytecode-graph-bulder to set the correct Hint for subtract operation.

BUG=v8:5273
LOG=N

Committed: https://crrev.com/26e2d16b8f1efef81790ca246f50c9fa24971f3b
Cr-Commit-Position: refs/heads/master@{#38467}
==========

[commit-bot: I haz the power, 2016-08-09 06:49:33]

Patchset 3 (id:??) landed as
https://crrev.com/26e2d16b8f1efef81790ca246f50c9fa24971f3b
Cr-Commit-Position: refs/heads/master@{#38467}