kernel->ssa: get simple constructors working

kernel->ssa: get simple constructors working

R=sigmund@google.com

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

[Harry Terkelsen, 2016-11-11 22:53:16]

het@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-11-14 18:22:25]

lgtm

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:131: /// Generative constructors
are built in several stages.
dartdoc nit: Consider making the first comment a description of the function
below, rather than a description of the algorithm?

/// Builds generative constructors.
///
/// Generative constructors are ...


another minor nit: "several" => "two"?

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:141: // TODO(het): Does kernel
always put field initializers in the constructor
good question for tomorrow :)?

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:166: hasRtiInput: false);
just checking - this might change when addressing the TODO above?

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:211: if (!foundSuperCall) {
I wonder if kernel injects this call always at the end in that case? Or if we
can consider making this normalization happen in kernel (TODO?)

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:272: void
_buildSuperConstructor(ir.Constructor constructor,
Consider renaming this to avoid giving the impression that we are creating the
actual super constructor function?

Some ideas:
 - _buildInlinedSuperConstructor
 - _buildInlinedSuperInitializers (in case we only intend to put initializer
code and nothing from the constructor bodies)

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/ke...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/ke...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:139: ir.Field
getFieldFromNode(FieldElement field) {
FromNode => FromElement? or simply getField?

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
File tests/compiler/dart2js/kernel/constructors_test.dart (right):

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
tests/compiler/dart2js/kernel/constructors_test.dart:20: return check(code,
checkElement: (Compiler compiler) {
nit: consider defining a helper function for this either here or in helper.dart.

defaultConstructorOf(String cls) => (compiler) {
  return compiler.mainApp.find(cls).lookupDefaultConstructor();
};

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
File tests/compiler/dart2js/kernel/helper.dart (right):

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
tests/compiler/dart2js/kernel/helper.dart:21: Element checkElement(Compiler
compiler)}) async {
maybe we can merge checkElement & entry, they seem to have a similar purpose. An
idea:

/// Returns the code associated with an element after compiling [code].
///
/// [lookup] defines how to find such element. It can be either a 
/// String with the name of a top-level element or an
/// [ElementLookup] callback that finds the appropriate element.
{dynamic lookup: 'main', ...}

typedef Element ElementLookup(Compiler compiler);

[Harry Terkelsen, 2016-11-14 23:23:40]

Thanks, Siggi!

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:131: /// Generative constructors
are built in several stages.
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> dartdoc nit: Consider making the first comment a description of the function
> below, rather than a description of the algorithm?
> 
> /// Builds generative constructors.
> ///
> /// Generative constructors are ...
> 
> 
> another minor nit: "several" => "two"?
> 

oops :) when I started that comment I thought there would be more phases

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:141: // TODO(het): Does kernel
always put field initializers in the constructor
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> good question for tomorrow :)?

Acknowledged.

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:166: hasRtiInput: false);
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> just checking - this might change when addressing the TODO above?

yes

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:211: if (!foundSuperCall) {
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> I wonder if kernel injects this call always at the end in that case? Or if we
> can consider making this normalization happen in kernel (TODO?)

Done.

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:272: void
_buildSuperConstructor(ir.Constructor constructor,
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> Consider renaming this to avoid giving the impression that we are creating the
> actual super constructor function?
> 
> Some ideas:
>  - _buildInlinedSuperConstructor
>  - _buildInlinedSuperInitializers (in case we only intend to put initializer
> code and nothing from the constructor bodies)

Done.

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/ke...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2498493003/diff/1/pkg/compiler/lib/src/ssa/ke...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:139: ir.Field
getFieldFromNode(FieldElement field) {
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> FromNode => FromElement? or simply getField?

Switched to FromElement. I didn't want to do getField since methods in this
class that are named 'getXXX' go from the Kernel version of XXX to the
Element/Node version.

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
File tests/compiler/dart2js/kernel/constructors_test.dart (right):

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
tests/compiler/dart2js/kernel/constructors_test.dart:20: return check(code,
checkElement: (Compiler compiler) {
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> nit: consider defining a helper function for this either here or in
helper.dart.
> 
> defaultConstructorOf(String cls) => (compiler) {
>   return compiler.mainApp.find(cls).lookupDefaultConstructor();
> };

Done.

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
File tests/compiler/dart2js/kernel/helper.dart (right):

https://codereview.chromium.org/2498493003/diff/1/tests/compiler/dart2js/kern...
tests/compiler/dart2js/kernel/helper.dart:21: Element checkElement(Compiler
compiler)}) async {
On 2016/11/14 18:22:25, Siggi Cherem (dart-lang) wrote:
> maybe we can merge checkElement & entry, they seem to have a similar purpose.
An
> idea:
> 
> /// Returns the code associated with an element after compiling [code].
> ///
> /// [lookup] defines how to find such element. It can be either a 
> /// String with the name of a top-level element or an
> /// [ElementLookup] callback that finds the appropriate element.
> {dynamic lookup: 'main', ...}
> 
> typedef Element ElementLookup(Compiler compiler);

Done.

[Harry Terkelsen, 2016-11-14 23:46:01]

Description was changed from

==========
kernel->ssa: get simple constructors working
==========

to

==========
kernel->ssa: get simple constructors working

R=sigmund@google.com

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

[Harry Terkelsen, 2016-11-14 23:46:02]

Committed patchset #2 (id:20001) manually as
de49cba5350e04cf21e796576541899e5a241279 (presubmit successful).