DBC: Collect type feedback for fastpath smi ops

DBC: Collect type feedback for fastpath smi ops

R=turnidge@google.com, vegorov@google.com

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

[Vyacheslav Egorov (Google), 2016-05-26 23:22:48]

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

[Vyacheslav Egorov (Google), 2016-05-26 23:22:48]

I think it's fine to say that fast path operations like AddTOS are always
followed by InstanceCall - so there is no need to add argument to AddTOS, but
rather you can just look ahead in the interpreter at pc[-1] and update the
ICData attached to the call.

[zra, 2016-05-27 17:08:07]

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

[zra, 2016-05-27 17:08:08]

PTAL

I've also fixed a crash caused when a source report with profile data is
requested in the observatory when profiling is disabled.

[zra, 2016-05-31 14:37:25]

ping

[Vyacheslav Egorov (Google), 2016-05-31 14:55:44]

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

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3074: }
maybe:

} else {
  return;
}

bool is_smi_two_args_op = call_ic_data->AddSmiSmiCheckForFastSmiStubs();
ASSERT(is_smi_two_args_op);

then big if above is not needed.

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:761: Bytecode::IsCallOpcode(*pc)) {                 
                        \
I think we should mandate that the next bytecode must be a call. 

Checking that next bytecode is call is too much branches (our fast pathes
already grow to big and convoluted).

[zra, 2016-05-31 16:33:12]

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

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3074: }
On 2016/05/31 14:55:44, Vyacheslav Egorov (Google) wrote:
> maybe:
> 
> } else {
>   return;
> }
> 
> bool is_smi_two_args_op = call_ic_data->AddSmiSmiCheckForFastSmiStubs();
> ASSERT(is_smi_two_args_op);
> 
> then big if above is not needed.

Done.

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:761: Bytecode::IsCallOpcode(*pc)) {                 
                        \
On 2016/05/31 14:55:44, Vyacheslav Egorov (Google) wrote:
> I think we should mandate that the next bytecode must be a call. 
> 
> Checking that next bytecode is call is too much branches (our fast pathes
> already grow to big and convoluted).

I'll make it mandatory in release builds, but optional in debug builds so that I
can retain the unit tests of the OpTOS instructions.

[Vyacheslav Egorov (Google), 2016-05-31 16:53:07]

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2015113002/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:761: Bytecode::IsCallOpcode(*pc)) {                 
                        \
On 2016/05/31 16:33:12, zra wrote:
> On 2016/05/31 14:55:44, Vyacheslav Egorov (Google) wrote:
> > I think we should mandate that the next bytecode must be a call. 
> > 
> > Checking that next bytecode is call is too much branches (our fast pathes
> > already grow to big and convoluted).
> 
> I'll make it mandatory in release builds, but optional in debug builds so that
I
> can retain the unit tests of the OpTOS instructions.

I am concerned that we will be unit testing the implementation that is not used.

I think we should be unit testing combination of XyzTOS + InstanceCall.

[zra, 2016-05-31 16:58:33]

On 2016/05/31 16:53:07, Vyacheslav Egorov (Google) wrote:
> I think we should be unit testing combination of XyzTOS + InstanceCall.

Please suggest how to generate an instance call in an assembler test. If there's
no good way to do this, then we'll probably just have to delete all the tests.

[turnidge, 2016-05-31 17:24:09]

lgtm for profiler_service.cc

[Vyacheslav Egorov (Google), 2016-06-01 09:25:14]

On 2016/05/31 16:58:33, zra wrote:
> On 2016/05/31 16:53:07, Vyacheslav Egorov (Google) wrote:
> > I think we should be unit testing combination of XyzTOS + InstanceCall.
> 
> Please suggest how to generate an instance call in an assembler test. If
there's
> no good way to do this, then we'll probably just have to delete all the tests.

Is it not possible to create and populate some fake ICData in the Assembler
test?

You can populate it with a cid, function pair that redirects call to some hand
written function - so the actual instance lookup code is not exercised?

[zra, 2016-06-01 21:22:43]

On 2016/06/01 09:25:14, Vyacheslav Egorov (Google) wrote:
> You can populate it with a cid, function pair that redirects call to some hand
> written function - so the actual instance lookup code is not exercised?

Done.

[Vyacheslav Egorov (Google), 2016-06-01 22:41:11]

lgtm

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/assembler_d...
File runtime/vm/assembler_dbc_test.cc (right):

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/assembler_d...
runtime/vm/assembler_dbc_test.cc:101: ic_data.AddCheck(cids,
dummy_instance_function);
Might be good to put something else - just to make sure it's not called.

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/object_dbc_...
File runtime/vm/object_dbc_test.cc (right):

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/object_dbc_...
runtime/vm/object_dbc_test.cc:19: void GenerateIncrement(Assembler* assembler) {
Add a comment that we don't want to generate increment for complexity reasons -
otherwise naming is too confusing.

[zra, 2016-06-03 16:08:57]

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/assembler_d...
File runtime/vm/assembler_dbc_test.cc (right):

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/assembler_d...
runtime/vm/assembler_dbc_test.cc:101: ic_data.AddCheck(cids,
dummy_instance_function);
On 2016/06/01 22:41:10, Vyacheslav Egorov (Google) wrote:
> Might be good to put something else - just to make sure it's not called.

Since we have to test the case where there is a non-Smi on the stack for an
operation, this is indeed called.

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/object_dbc_...
File runtime/vm/object_dbc_test.cc (right):

https://codereview.chromium.org/2015113002/diff/100001/runtime/vm/object_dbc_...
runtime/vm/object_dbc_test.cc:19: void GenerateIncrement(Assembler* assembler) {
On 2016/06/01 22:41:10, Vyacheslav Egorov (Google) wrote:
> Add a comment that we don't want to generate increment for complexity reasons
-
> otherwise naming is too confusing.

Done.

[zra, 2016-06-03 16:09:16]

Description was changed from

==========
DBC: Collect type feedback for fastpath smi ops
==========

to

==========
DBC: Collect type feedback for fastpath smi ops

R=turnidge@google.com, vegorov@google.com

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

[zra, 2016-06-03 16:09:17]

Committed patchset #7 (id:120001) manually as
de78ddbc555416155f63634ba388cd8c1d26a229 (presubmit successful).