[interpreter] Introduce binary op bytecodes for Smi operand.

[interpreter] Introduce binary op bytecodes for Smi operand.

Introduces fused bytecodes for fusing LdaSmi followed by a binary op bytecode.
The chosen bytecodes are used frequently in Octane: AddSmi, SubSmi,
BitwiseOrSmi, BitwiseAndSmi, ShiftLeftSmi, ShiftRightSmi.

There are additional code stubs for these operations that are biased towards
both the left hand and right hand operands being Smis.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/40511877eb2f3c7fdab2d0dd86cc9668e5454cb7
Cr-Commit-Position: refs/heads/master@{#37531}

[oth, 2016-06-30 13:27:11]

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

[oth, 2016-06-30 13:27:11]

Benedikt, just kicking the tires with this to confirm whether this is an awful
idea?

Basically spotting that the rhs of a binary op is a smi in bytecode and then
using a custom inline stub to handle it.  It seems to give a bit of perf step
over using the existing inline stubs for these operators.

WDYT?

Thanks

[Benedikt Meurer, 2016-07-01 04:53:17]

Great idea, I like it! Should also help to reduce bytecode size in numeric code,
right? Do you also consider commutativity?

[oth, 2016-07-01 09:11:40]

On 2016/07/01 04:53:17, Benedikt Meurer wrote:
> Great idea, I like it! Should also help to reduce bytecode size in numeric
code,
> right? Do you also consider commutativity?

Thanks. I'll clean this up and then get it out for review.

There is a saving in size and it's generally positive-neutral on interpreted
Octane for X64/ARM (total 1-2% higher), IA32 is ~0.5% slower (TBD).

For the commutative case, we don't have an effective means of detecting whether
the LHS is a SMI as the binary op bytecodes take the LHS from an interpreter
register. I suspect code is biased towards SMI on the RHS so hopefully catch the
common case already.

[oth, 2016-07-04 10:20:14]

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

[oth, 2016-07-04 10:20:15]

Ross, PTAL.

I've not completed writing tests here. If it looks okay, this'll happen and I'll
loop in Benedikt for a review of stubs and compiler/.

We probably need to pick which peephole optimization we want to take here - this
or your upcoming binary op plus store accumulator. The perf gains from either
are likely quite small either way.

Thanks

[rmcilroy, 2016-07-04 12:35:32]

Looks good, a couple of optional suggestions but otherwise interpreter stuff
LGTM when tests are added tests.

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

https://codereview.chromium.org/2111923002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:897: // {right} is known to be a Smi.
Maybe add some code under FLAG_debug_code which aborts if rhs isn't a SMI)?

https://codereview.chromium.org/2111923002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:915: assembler->Bind(&slowpath);
I wonder if it is worth doing the check for heap-number here too (and the
overflow addition) before going to the slow path, (i.e., the equivalent code as
is in IncStub). If we did that then we could possibly replace IncStub with
SubSmiStub(val, SmiConstant(-1)); and DecStub with SubSmiStub(val,
SmiConstant(1));

Just a thought, and don't want to block this CL if you've already considered
this.

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

https://codereview.chromium.org/2111923002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:129: return current->bytecode()
== Bytecode::kNop;
Can we do this in a separate CL please, so we can see the impact of this change
seperately from the SMI rhs

https://codereview.chromium.org/2111923002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:264: } else if (last_.bytecode()
== Bytecode::kLdaZero &&
Can we not also do the LdaZero optimization for all cases above too?

https://codereview.chromium.org/2111923002/diff/40001/test/unittests/interpre...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2111923002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:70: // Force
emission of AddSmi.
nit - maybe just group these all together since they are simlar peephole
operations?

https://codereview.chromium.org/2111923002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:385:
the_array->Print();
leftover debugging?

[oth, 2016-07-04 14:56:24]

Thanks for the feedback. Comments incorporated. Test to follow.

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

https://codereview.chromium.org/2111923002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:897: // {right} is known to be a Smi.
On 2016/07/04 12:35:32, rmcilroy wrote:
> Maybe add some code under FLAG_debug_code which aborts if rhs isn't a SMI)?

Done. CodeStubAssembler::Assert is conditional on DEBUG macro which seems
reasonable.

https://codereview.chromium.org/2111923002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:915: assembler->Bind(&slowpath);
On 2016/07/04 12:35:32, rmcilroy wrote:
> I wonder if it is worth doing the check for heap-number here too (and the
> overflow addition) before going to the slow path, (i.e., the equivalent code
as
> is in IncStub). If we did that then we could possibly replace IncStub with
> SubSmiStub(val, SmiConstant(-1)); and DecStub with SubSmiStub(val,
> SmiConstant(1));
> 
> Just a thought, and don't want to block this CL if you've already considered
> this.

Yes, I tried this. For x64 the stub size increases from 227 bytes to 498 bytes
(compared to 1820 bytes for regular add). At least in Octane there's not a
discernible perf win from having this path. It's an easy change if you'd prefer
it to be present.

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

https://codereview.chromium.org/2111923002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:129: return current->bytecode()
== Bytecode::kNop;
On 2016/07/04 12:35:32, rmcilroy wrote:
> Can we do this in a separate CL please, so we can see the impact of this
change
> seperately from the SMI rhs

Done.

https://codereview.chromium.org/2111923002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:264: } else if (last_.bytecode()
== Bytecode::kLdaZero &&
On 2016/07/04 12:35:32, rmcilroy wrote:
> Can we not also do the LdaZero optimization for all cases above too?

Done. This was skipped due to low numbers of occurrences in Octane, but not a
significant tax.

https://codereview.chromium.org/2111923002/diff/40001/test/unittests/interpre...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2111923002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:70: // Force
emission of AddSmi.
On 2016/07/04 12:35:32, rmcilroy wrote:
> nit - maybe just group these all together since they are simlar peephole
> operations?

Done.

https://codereview.chromium.org/2111923002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:385:
the_array->Print();
On 2016/07/04 12:35:32, rmcilroy wrote:
> leftover debugging?

Yep, done. :-)

[oth, 2016-07-04 15:48:49]

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

[oth, 2016-07-04 15:48:50]

bmeurer@chromium.org: Please review changes in code-stubs and compiler now it's
a bit cleaner. Thanks!

[Benedikt Meurer, 2016-07-05 03:36:23]

Please don't put those operations into code stubs. They will never be used as
stubs, so just inline them directly into the interpreter.

[oth, 2016-07-05 08:09:41]

On 2016/07/05 03:36:23, Benedikt Meurer wrote:
> Please don't put those operations into code stubs. They will never be used as
> stubs, so just inline them directly into the interpreter.

Done, good point. At some stage they had more of the existing stubs dropped in
and had been thinking it'd be easier to maintain if side-by-side, but now they
use so little this isn't a good reason.

PTAL at bytecode-graph-builder, thanks.

[Benedikt Meurer, 2016-07-05 09:03:26]

Thanks, LGTM from my side.

[rmcilroy, 2016-07-05 09:55:41]

Still LGTM, thanks.

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

https://codereview.chromium.org/2111923002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:868: __ Goto(&end);
Might be worth dispatching directly here - a dispatch which happens before any
Calls have happened is slightly faster than otherwise (no need to re-load
bytecode array) - might not have any impact though, just a thought.

[oth, 2016-07-05 13:19:35]

Thanks!

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

https://codereview.chromium.org/2111923002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:868: __ Goto(&end);
On 2016/07/05 09:55:41, rmcilroy wrote:
> Might be worth dispatching directly here - a dispatch which happens before any
> Calls have happened is slightly faster than otherwise (no need to re-load
> bytecode array) - might not have any impact though, just a thought.

Yes though for interpreted Octane on desktop, the perf doesn't seem to increase
above the noise. Inclined to leave as-is for now and see if it shows up when
profiling bytecode handlers.

[oth, 2016-07-05 13:20:24]

The CQ bit was checked by oth@chromium.org

[oth, 2016-07-05 13:20:24]

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/2111923002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-05 13:20:29]

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-05 13:44:14]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-05 13:46:23]

Description was changed from

==========
[interpreter] Introduce binary op bytecodes for Smi operand.

Introduces fused bytecodes for fusing LdaSmi followed by a binary op bytecode.
The chosen bytecodes are used frequently in Octane: AddSmi, SubSmi,
BitwiseOrSmi, BitwiseAndSmi, ShiftLeftSmi, ShiftRightSmi.

There are additional code stubs for these operations that are biased towards
both the left hand and right hand operands being Smis.

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

to

==========
[interpreter] Introduce binary op bytecodes for Smi operand.

Introduces fused bytecodes for fusing LdaSmi followed by a binary op bytecode.
The chosen bytecodes are used frequently in Octane: AddSmi, SubSmi,
BitwiseOrSmi, BitwiseAndSmi, ShiftLeftSmi, ShiftRightSmi.

There are additional code stubs for these operations that are biased towards
both the left hand and right hand operands being Smis.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/40511877eb2f3c7fdab2d0dd86cc9668e5454cb7
Cr-Commit-Position: refs/heads/master@{#37531}
==========

[commit-bot: I haz the power, 2016-07-05 13:46:25]

Patchset 8 (id:??) landed as
https://crrev.com/40511877eb2f3c7fdab2d0dd86cc9668e5454cb7
Cr-Commit-Position: refs/heads/master@{#37531}