[VM DBC compiler and simulator] Support reified generic functions.

[VM DBC compiler and simulator] Support reified generic functions.

R=vegorov@google.com

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

[regis, 2017-08-01 22:36:40]

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

[regis, 2017-08-01 22:36:40]

This is not ready for submission, because it only works in non-optimized code.
In optimized mode, the register holding the type arguments is wrongly reused by
the graph allocator for a reason I have not yet understood. Since I'll be off
for the rest of the week, I thought I would send what I have so far, so you can
have a look.

Thanks,
Regis

[Vyacheslav Egorov (Google), 2017-08-08 17:44:43]

Hi Regis! 

I was not sure which test to run to reproduce the problem, but I think I know
what the problem is.

Please see my comment below.

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/flow_graph_alloc...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:723: if (range->End() > kNormalEntryPos) {
I think you need this code (lines 723 - line 728) duplicated after

  735     range->set_assigned_location(Location::RegisterLocation(slot_index));

this should prevent the register from being reused.

[zra, 2017-08-08 17:47:18]

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/flow_graph_alloc...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:637: #if defined(TARGET_ARCH_DBC)
A pretty big portion of this function is now under "#if
defined(TARGET_ARCH_DBC)" I wonder if it would be easier to read if we did
something like:

#if defined(TARGET_ARCH_DBC)
void FlowGraphAllocator::ProcessInitialDefinition() {
  // dbc version
}
#else
void FlowGraphAllocator::ProcessInitialDefinition() {
  // regular version
}
#endif

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/simulator_dbc.cc
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:884: const intptr_t type_args_len =
Smi::Value(*reinterpret_cast<RawSmi**>(
Maybe pull the type_args_len calculation out into SimulatorHelpers instead of
duplicating it.

[regis, 2017-08-08 22:58:28]

Thank you both!

PTAL

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/flow_graph_alloc...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:637: #if defined(TARGET_ARCH_DBC)
On 2017/08/08 17:47:17, zra wrote:
> A pretty big portion of this function is now under "#if
> defined(TARGET_ARCH_DBC)" I wonder if it would be easier to read if we did
> something like:
> 
> #if defined(TARGET_ARCH_DBC)
> void FlowGraphAllocator::ProcessInitialDefinition() {
>   // dbc version
> }
> #else
> void FlowGraphAllocator::ProcessInitialDefinition() {
>   // regular version
> }
> #endif

Done. It is now easier to read, but harder to maintain, because of the code
duplication. I am fine with either versions and do not mind to revert if Slava
prefers maintainability over readability.

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:723: if (range->End() > kNormalEntryPos) {
On 2017/08/08 17:44:42, Vyacheslav Egorov (Google) wrote:
> I think you need this code (lines 723 - line 728) duplicated after
> 
>   735    
range->set_assigned_location(Location::RegisterLocation(slot_index));
> 
> this should prevent the register from being reused. 

Done. Awesome! That did it! Thanks.

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/simulator_dbc.cc
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2987323003/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:884: const intptr_t type_args_len =
Smi::Value(*reinterpret_cast<RawSmi**>(
On 2017/08/08 17:47:18, zra wrote:
> Maybe pull the type_args_len calculation out into SimulatorHelpers instead of
> duplicating it.

Done.

[Vyacheslav Egorov (Google), 2017-08-13 13:19:23]

overall LGTM but I would like us to find a readable form for
ProcessInitialDefinitions

https://codereview.chromium.org/2987323003/diff/40001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/40001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:634: #if defined(TARGET_ARCH_DBC)
I think I liked original version of the function that was not split more.

In the current version there is a lot of duplicated code.

https://codereview.chromium.org/2987323003/diff/40001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:710: AssignSafepoints(defn, range);
I think lines 710-717 and 724-729 should be almost the same with the only
difference that we assign different location to the range. 

Please refactor this accordingly to avoid duplicating code.

[zra, 2017-08-14 19:59:06]

https://codereview.chromium.org/2987323003/diff/40001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/40001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:634: #if defined(TARGET_ARCH_DBC)
On 2017/08/13 13:19:23, Vyacheslav Egorov (Google) wrote:
> I think I liked original version of the function that was not split more.
> 
> In the current version there is a lot of duplicated code.

I'll defer to Slava here.

[regis, 2017-08-14 21:59:31]

I improved code factorization in a single version of ProcessInitialDefinition().
Please, let me know what you think.

I still need to figure out and fix the last failure before committing:

out/DebugSIMDBC64/dart --reify-generic-functions 
tests/lib/convert/encoding_test.dart
unittest-suite-wait-for-done
../../runtime/vm/raw_object.h: 1658: error: expected: IsHeapObject()

Thanks,
Regis

[regis, 2017-08-14 23:29:19]

https://codereview.chromium.org/2987323003/diff/40001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/40001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:710: AssignSafepoints(defn, range);
On 2017/08/13 13:19:23, Vyacheslav Egorov (Google) wrote:
> I think lines 710-717 and 724-729 should be almost the same with the only
> difference that we assign different location to the range. 
> 
> Please refactor this accordingly to avoid duplicating code.

Done.

https://codereview.chromium.org/2987323003/diff/60001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/60001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:717: slot_index++;
If I remove this special handling on lines 712-718, i.e. if I never increment
slot_index, all tests pass. I do not understand why the context variable does
not collide with the type argument variable.

[Vyacheslav Egorov (Google), 2017-08-15 12:31:01]

https://codereview.chromium.org/2987323003/diff/60001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/60001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:717: slot_index++;
On 2017/08/14 23:29:19, regis wrote:
> If I remove this special handling on lines 712-718, i.e. if I never increment
> slot_index, all tests pass. I do not understand why the context variable does
> not collide with the type argument variable.

Actually if you change stuff here you need also to change code in
runtime/vm/flow_graph_compiler_dbc.cc line 385 and further. They must match what
happens here. 

I am surprised it passes with storing context into completely different slot. 

Maybe there is no test that exercises both simultaneously?

[regis, 2017-08-15 22:01:53]

This cl is now ready for submission.

Thanks!

https://codereview.chromium.org/2987323003/diff/60001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2987323003/diff/60001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:717: slot_index++;
On 2017/08/15 12:31:00, Vyacheslav Egorov (Google) wrote:
> On 2017/08/14 23:29:19, regis wrote:
> > If I remove this special handling on lines 712-718, i.e. if I never
increment
> > slot_index, all tests pass. I do not understand why the context variable
does
> > not collide with the type argument variable.
> 
> Actually if you change stuff here you need also to change code in
> runtime/vm/flow_graph_compiler_dbc.cc line 385 and further. They must match
what
> happens here. 
> 
> I am surprised it passes with storing context into completely different slot. 
> 
> Maybe there is no test that exercises both simultaneously?

That was it. Thanks!
Yes, we have very few tests for generic functions, since only DDC is currently
supporting them.

[Vyacheslav Egorov (Google), 2017-08-16 05:27:36]

LGTM

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

https://codereview.chromium.org/2987323003/diff/80001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:392: is_optimizing() ?
((parsed_function().function_type_arguments() != NULL)
The code below uses FLAG_reify_generic_functions && function.IsGeneric() and
here we use (parsed_function().function_type_arguments() != NULL).

Is it possible to unify these conditions?

Maybe even make

const bool has_type_arguments = ...

[regis, 2017-08-16 17:47:48]

Thanks!

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

https://codereview.chromium.org/2987323003/diff/80001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:392: is_optimizing() ?
((parsed_function().function_type_arguments() != NULL)
On 2017/08/16 05:27:36, Vyacheslav Egorov (Google) wrote:
> The code below uses FLAG_reify_generic_functions && function.IsGeneric() and
> here we use (parsed_function().function_type_arguments() != NULL).
> 
> Is it possible to unify these conditions?
> 
> Maybe even make
> 
> const bool has_type_arguments = ...

Done.
I also unified with the code in FlowGraphAllocator::ProcessInitialDefinition.

[regis, 2017-08-16 17:48:11]

Description was changed from

==========
[VM DBC compiler and simulator] Support reified generic functions.
==========

to

==========
[VM DBC compiler and simulator] Support reified generic functions.

R=vegorov@google.com

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

[regis, 2017-08-16 17:48:13]

Committed patchset #6 (id:100001) manually as
c29f5dd4f538fb1005f193d012028229a523de4c (presubmit successful).