DBC: Adds BinarySmiOp instruction

DBC: Adds BinarySmiOp instruction

R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/b3e5c5eeed62bf59740ef9a65175da650f3942f9

[zra, 2016-06-10 21:57:07]

zra@google.com changed reviewers:
+ vegorov@google.com

[zra, 2016-06-10 21:57:08]

Took a stab at this. When the structure is nailed down, I'll add docs and tests
for the new instructions.

[Vyacheslav Egorov (Google), 2016-06-11 08:09:47]

https://codereview.chromium.org/2053213004/diff/1/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/2053213004/diff/1/runtime/vm/constants_dbc.h#...
runtime/vm/constants_dbc.h:426: V(AddChecked,                  A_B_C, reg, reg,
reg) \
an interesting question is whether having unchecked vs checked versions pays of
in the interpreted mode.

could you try benchmarking?

It might be that overhead of interpretation hides away overhead of checking for
overflow. In this case maybe it's simpler to just have a checked version and
emit unchecked version as 

Add a, b, c
Nop

[zra, 2016-06-13 21:11:24]

PTAL

https://codereview.chromium.org/2053213004/diff/1/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/2053213004/diff/1/runtime/vm/constants_dbc.h#...
runtime/vm/constants_dbc.h:426: V(AddChecked,                  A_B_C, reg, reg,
reg) \
On 2016/06/11 08:09:47, Vyacheslav Egorov (Google) wrote:
> an interesting question is whether having unchecked vs checked versions pays
of
> in the interpreted mode.
> 
> could you try benchmarking?
> 
> It might be that overhead of interpretation hides away overhead of checking
for
> overflow. In this case maybe it's simpler to just have a checked version and
> emit unchecked version as 
> 
> Add a, b, c
> Nop 

In microbenchmarks, the unchecked versions were not clearly better. Maybe 1-2%.
So I removed them. Benchmarking on arm64 uncovered a bug in the constraints for
the inline assembly for the checked multiply.

[zra, 2016-06-15 06:20:46]

ping

[Vyacheslav Egorov (Google), 2016-06-15 06:44:51]

lgtm

https://codereview.chromium.org/2053213004/diff/20001/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/2053213004/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:166: //    If these instructions can trigger a
deoptimization, the following
Add a comment that these instructions expect their operands to be Smis and do
not check this. (because it's not obvious from their names). Same below.

https://codereview.chromium.org/2053213004/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:403: V(Nop,                             0, ___, ___,
___) \
This instruction is not documented.

[zra, 2016-06-15 21:47:37]

I hit some bugs, primarily with AssertAssignableInstr, and included fixes in
this CL.

https://codereview.chromium.org/2053213004/diff/20001/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/2053213004/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:166: //    If these instructions can trigger a
deoptimization, the following
On 2016/06/15 06:44:51, Vyacheslav Egorov (Google) wrote:
> Add a comment that these instructions expect their operands to be Smis and do
> not check this. (because it's not obvious from their names). Same below.

Done.

https://codereview.chromium.org/2053213004/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:403: V(Nop,                             0, ___, ___,
___) \
On 2016/06/15 06:44:50, Vyacheslav Egorov (Google) wrote:
> This instruction is not documented.

Done.

[zra, 2016-06-15 21:48:15]

Description was changed from

==========
DBC: Adds BinarySmiOp instruction
==========

to

==========
DBC: Adds BinarySmiOp instruction

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b3e5c5eeed62bf59740ef9a65175da650f3942f9
==========

[zra, 2016-06-15 21:48:17]

Committed patchset #3 (id:40001) manually as
b3e5c5eeed62bf59740ef9a65175da650f3942f9 (presubmit successful).

[Vyacheslav Egorov (Google), 2016-06-16 08:31:53]

https://codereview.chromium.org/2053213004/diff/40001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/2053213004/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:242: __ PopLocal(locs->out(0).reg());
I don't get this comment. GC can't have anything to do with this reload because
all live registers must be scanned. If in(0) is not scanned - it's a bug in
stack maps and it needs to be fixed.

https://codereview.chromium.org/2053213004/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2053213004/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:918: __ Push(locs()->in(0).reg());
CheckClassId should also probably be a single instructions like CheckSmi