Add ConstructorBodyEntity

Add ConstructorBodyEntity

+ remove KernelSsaGraphBuilder.astAdapter.

R=sigmund@google.com

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

[Johnni Winther, 2017-07-07 12:57:04]

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

[Siggi Cherem (dart-lang), 2017-07-07 20:00:26]

I have a big question about whether or not it should exist on the K-model. I
thought we didn't want it there.

Also a suggestion that should hopefully simplify what we do for deferred loaded
libraries.

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/entities.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/entities.dart:213: abstract class
ConstructorBodyEntity extends FunctionEntity {
+TODO or clarify that this entity shouldn't exist once we delete the old modelx?
(if we get rid of it in the K model)

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/namer.dart:786: // codegen model should be
explicit about its constructor body elements.
I guess this TODO no longer applies, right?

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_mod...
File pkg/compiler/lib/src/js_model/closure.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_mod...
pkg/compiler/lib/src/js_model/closure.dart:151: entity =
constructorBody.constructor;
it might be nice to do this at the call site though.

Don't we know at the callsite whether we are in a ctor body and should be using
the ctor as a key?

Also - this depends on what we decide about the K constructorbodyentities: if we
say that they are created later and they don't exit by the time we do closure
conversion, then  it feels odd that the closure task has to be aware of them at
all.

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:918: return new
KConstructorBody(memberIndex, constructor);
I thought we didn't need to have one on the K model?

If we do, I'd intead store in the closure-task the body as a key and only fetch
the original constructor in order to get a hold of the internal ir of the ctor
body.

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

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:750: String uri =
_elementMap.getDeferredUri(checkLoad.import);
I think in this case we don't need the extra method in elementMap. We can get
the uri directly from the IR as follows:

  String uri = checkLoad.import.targetLibrary.importUri.toString();

Or even change the next line as well to get better types:

  Uri uri = checkLoad.import.targetLibrary.importUri;
  HInstruction uriConstant = graph.addConstantString('$uri', closedWorld);

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/use.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/use.dart:249: FunctionEntity element,
CallStructure callStructure) {
why not ConsutrctorBodyEntity?

[Johnni Winther, 2017-07-10 11:27:57]

On 2017/07/07 20:00:26, Siggi Cherem (dart-lang) wrote:
> I have a big question about whether or not it should exist on the K-model. I
> thought we didn't want it there.
> 

Long-term we should create constructor bodies eagerly with the J-model, and they
should just be function entities. For now, to be consistent with the old model,
we cannot assume eager creation and thus need to do the annoying `is
ConstructorBodyEntity` occasionally. 

After rebasing onto - the now landed -
https://codereview.chromium.org/2974633002/, we don't need the KConstructorBody
class.

[Johnni Winther, 2017-07-10 14:11:34]

PTAL

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/entities.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/entities.dart:213: abstract class
ConstructorBodyEntity extends FunctionEntity {
On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> +TODO or clarify that this entity shouldn't exist once we delete the old
modelx?
> (if we get rid of it in the K model)

Done.

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/namer.dart:786: // codegen model should be
explicit about its constructor body elements.
On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> I guess this TODO no longer applies, right?

It still does, but not until after the redemption.

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_mod...
File pkg/compiler/lib/src/js_model/closure.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/js_mod...
pkg/compiler/lib/src/js_model/closure.dart:151: entity =
constructorBody.constructor;
On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> it might be nice to do this at the call site though.
> 
> Don't we know at the callsite whether we are in a ctor body and should be
using
> the ctor as a key?
> 
> Also - this depends on what we decide about the K constructorbodyentities: if
we
> say that they are created later and they don't exit by the time we do closure
> conversion, then  it feels odd that the closure task has to be aware of them
at
> all.

Long-term constructor body should have there own associated
ClosureRepresentationInfo. Adding a TODO.

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:918: return new
KConstructorBody(memberIndex, constructor);
On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> I thought we didn't need to have one on the K model?
> 
> If we do, I'd intead store in the closure-task the body as a key and only
fetch
> the original constructor in order to get a hold of the internal ir of the ctor
> body.

Now, we don't. [getConstructorBody], [_getConstructorBody] and
[createConstructorBody] are moved from [KernelToElementMapBase] to
[JsKernelToElementMap].

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

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:750: String uri =
_elementMap.getDeferredUri(checkLoad.import);
On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> I think in this case we don't need the extra method in elementMap. We can get
> the uri directly from the IR as follows:
> 
>   String uri = checkLoad.import.targetLibrary.importUri.toString();
> 
> Or even change the next line as well to get better types:
> 
>   Uri uri = checkLoad.import.targetLibrary.importUri;
>   HInstruction uriConstant = graph.addConstantString('$uri', closedWorld);

They're not the same in rasta :(

Added a TODO on getDeferredUri to resolve this.

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/use.dart (right):

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/use.dart:249: FunctionEntity element,
CallStructure callStructure) {
On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> why not ConsutrctorBodyEntity?

Changed to ConstructorBodyEntity (when I started the CL I had hoped to avoid
ConstructorBodyEntity)

[Siggi Cherem (dart-lang), 2017-07-10 16:39:13]

lgtm, glad to see the KConstructorBody go away.

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

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:750: String uri =
_elementMap.getDeferredUri(checkLoad.import);
On 2017/07/10 14:11:34, Johnni Winther wrote:
> On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> > I think in this case we don't need the extra method in elementMap. We can
get
> > the uri directly from the IR as follows:
> > 
> >   String uri = checkLoad.import.targetLibrary.importUri.toString();
> > 
> > Or even change the next line as well to get better types:
> > 
> >   Uri uri = checkLoad.import.targetLibrary.importUri;
> >   HInstruction uriConstant = graph.addConstantString('$uri', closedWorld);
> 
> They're not the same in rasta :(
> 
> Added a TODO on getDeferredUri to resolve this.

mmm - what is the actual difference? Is it that we used to get the Uri as it
appeared in the import statement, but rasta is giving us the resolved Uri? If
so, I'm guessing that the future front-end will behave like rasta too. I'm
guessing the solution is to use the resolve Uri here as well, correct?

I agree with doing that on a separate CL, but I don't know if there is more to
it than just replacing one Uri for another.

[Johnni Winther, 2017-07-11 08:09:17]

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

https://codereview.chromium.org/2968383002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:750: String uri =
_elementMap.getDeferredUri(checkLoad.import);
On 2017/07/10 16:39:13, Siggi Cherem (dart-lang) wrote:
> On 2017/07/10 14:11:34, Johnni Winther wrote:
> > On 2017/07/07 20:00:25, Siggi Cherem (dart-lang) wrote:
> > > I think in this case we don't need the extra method in elementMap. We can
> get
> > > the uri directly from the IR as follows:
> > > 
> > >   String uri = checkLoad.import.targetLibrary.importUri.toString();
> > > 
> > > Or even change the next line as well to get better types:
> > > 
> > >   Uri uri = checkLoad.import.targetLibrary.importUri;
> > >   HInstruction uriConstant = graph.addConstantString('$uri', closedWorld);
> > 
> > They're not the same in rasta :(
> > 
> > Added a TODO on getDeferredUri to resolve this.
> 
> mmm - what is the actual difference? Is it that we used to get the Uri as it
> appeared in the import statement, but rasta is giving us the resolved Uri? If
> so, I'm guessing that the future front-end will behave like rasta too. I'm
> guessing the solution is to use the resolve Uri here as well, correct?
> 
> I agree with doing that on a separate CL, but I don't know if there is more to
> it than just replacing one Uri for another.

One is relative and one is absolute.

I think long-term we will use the 'importUri' from .dill as the canonical
values.

[Johnni Winther, 2017-07-11 08:20:15]

Description was changed from

==========
Add ConstructorBodyEntity

+ remove KernelSsaGraphBuilder.astAdapter.
==========

to

==========
Add ConstructorBodyEntity

+ remove KernelSsaGraphBuilder.astAdapter.

R=sigmund@google.com

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

[Johnni Winther, 2017-07-11 08:20:17]

Committed patchset #4 (id:60001) manually as
ea7b7f3e57118fde22ba7aba0b870d937aa98868 (presubmit successful).