Add types for map literal construction with kernel code.

Add types for map literal construction with kernel code.

Possible this could be further simplified?

.... http://i1.kym-cdn.com/photos/images/facebook/000/234/739/fa5.jpg

BUG=https://github.com/dart-lang/sdk/issues/27394
R=sigmund@google.com, sra@google.com

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

[Emily Fortuna, 2016-11-17 23:07:59]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2016-11-17 23:08:45]

efortuna@google.com changed reviewers:
+ johnniwinther@google.com, sra@google.com

[sra1, 2016-11-17 23:51:41]

lgtm

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:723: /// (3) subclasses of (2) and
(3).
The code is now a little bit too general, since it is called only for JSArray<E>
We could need JSArray<E> for the reasons (1)...(3).

[Siggi Cherem (dart-lang), 2016-11-17 23:56:53]

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

[Siggi Cherem (dart-lang), 2016-11-17 23:56:53]

lgtm

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:837: // TODO(efortuna): Inlining.
Let's change the TODO to explicitly say this is for
'currentInlinedInstantiations'?

I think "inlined" there is a misnomer. Maybe it should be called "implicit"
instantiations? The thing is that this is used even when we do no inlining. It's
used to correctly register what types are instantiated from factory
constructors, and hence to know what else to include when tree-shaking. So we'll
need to have this sooner rather than later.

[Emily Fortuna, 2016-11-18 01:29:33]

Patchset #2 (id:40001) has been deleted

[Emily Fortuna, 2016-11-18 01:30:35]

I made the commented changes, but PTAL. I'm really doubtful that the
implicitInstantiations is doing what we want it to do.

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:723: /// (3) subclasses of (2) and
(3).
On 2016/11/17 23:51:41, sra1 wrote:
> The code is now a little bit too general, since it is called only for
JSArray<E>
> We could need JSArray<E> for the reasons (1)...(3).

Done.

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:837: // TODO(efortuna): Inlining.
On 2016/11/17 23:56:53, Siggi Cherem (dart-lang) wrote:
> Let's change the TODO to explicitly say this is for
> 'currentInlinedInstantiations'?
> 
> I think "inlined" there is a misnomer. Maybe it should be called "implicit"
> instantiations? The thing is that this is used even when we do no inlining.
It's
> used to correctly register what types are instantiated from factory
> constructors, and hence to know what else to include when tree-shaking. So
we'll
> need to have this sooner rather than later.

hmmmm. I added them, but I'm not certain that this does what we want, since we
already have the type itself and don't have to follow it like we do in
builder.dart (see line 5676). The actual type is already passed into
_pushStaticInvocation(instructionType). So I'm not sure why we need to push it
onto that stack as well? I'm inclined to take the added bits of the second
patchset out and also remove the above TODO then. WDYT? Am I not thinking of
some particular use case?

[Siggi Cherem (dart-lang), 2016-11-18 15:47:07]

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:837: // TODO(efortuna): Inlining.
On 2016/11/18 01:30:35, Emily Fortuna wrote:
> On 2016/11/17 23:56:53, Siggi Cherem (dart-lang) wrote:
> > Let's change the TODO to explicitly say this is for
> > 'currentInlinedInstantiations'?
> > 
> > I think "inlined" there is a misnomer. Maybe it should be called "implicit"
> > instantiations? The thing is that this is used even when we do no inlining.
> It's
> > used to correctly register what types are instantiated from factory
> > constructors, and hence to know what else to include when tree-shaking. So
> we'll
> > need to have this sooner rather than later.
> 
> hmmmm. I added them, but I'm not certain that this does what we want, since we
> already have the type itself and don't have to follow it like we do in
> builder.dart (see line 5676). The actual type is already passed into
> _pushStaticInvocation(instructionType).


Stephen/Johnni - please confirm, but I believe we also need them for regular
factory constructors (not just for redirecting constructors).

If I understand this correctly, when we see:

   x = {}

We generate a static call to the factory method:

   factory LinkedHashMap._empty() {
     return new JsLinkedHashMap.es6();
   }

The call doesn't know below that it is a factory method, so even though we will
mark JsLinkedHashMap as instantiated when looking at the body of the method, we
wont include LinkedHashMap unless we add this implicit instantiation.

> So I'm not sure why we need to push it
> onto that stack as well? I'm inclined to take the added bits of the second
> patchset out and also remove the above TODO then. WDYT? Am I not thinking of
> some particular use case?

[Emily Fortuna, 2016-11-18 18:40:58]

On 2016/11/18 15:47:07, Siggi Cherem (dart-lang) wrote:
>
https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
> File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):
> 
>
https://codereview.chromium.org/2512023002/diff/20001/pkg/compiler/lib/src/ss...
> pkg/compiler/lib/src/ssa/builder_kernel.dart:837: // TODO(efortuna): Inlining.
> On 2016/11/18 01:30:35, Emily Fortuna wrote:
> > On 2016/11/17 23:56:53, Siggi Cherem (dart-lang) wrote:
> > > Let's change the TODO to explicitly say this is for
> > > 'currentInlinedInstantiations'?
> > > 
> > > I think "inlined" there is a misnomer. Maybe it should be called
"implicit"
> > > instantiations? The thing is that this is used even when we do no
inlining.
> > It's
> > > used to correctly register what types are instantiated from factory
> > > constructors, and hence to know what else to include when tree-shaking. So
> > we'll
> > > need to have this sooner rather than later.
> > 
> > hmmmm. I added them, but I'm not certain that this does what we want, since
we
> > already have the type itself and don't have to follow it like we do in
> > builder.dart (see line 5676). The actual type is already passed into
> > _pushStaticInvocation(instructionType).
> 
> 
> Stephen/Johnni - please confirm, but I believe we also need them for regular
> factory constructors (not just for redirecting constructors).
> 
> If I understand this correctly, when we see:
> 
>    x = {}
> 
> We generate a static call to the factory method:
> 
>    factory LinkedHashMap._empty() {
>      return new JsLinkedHashMap.es6();
>    }
> 
> The call doesn't know below that it is a factory method, so even though we
will
> mark JsLinkedHashMap as instantiated when looking at the body of the method,
we
> wont include LinkedHashMap unless we add this implicit instantiation.
> 
> > So I'm not sure why we need to push it
> > onto that stack as well? I'm inclined to take the added bits of the second
> > patchset out and also remove the above TODO then. WDYT? Am I not thinking of
> > some particular use case?

We (Siggi, Stephen and I) talked offline about this and the conclusion was
without the addition of this code, it might accidentally do the correct thing
already, but to be safe, we'll keep the stack, which will also be used for the
plain factory constructor use case.

[Emily Fortuna, 2016-11-18 18:41:33]

Description was changed from

==========
Add types for map literal construction with kernel code.

Possible this could be further simplified?

.... http://i1.kym-cdn.com/photos/images/facebook/000/234/739/fa5.jpg

BUG=https://github.com/dart-lang/sdk/issues/27394
==========

to

==========
Add types for map literal construction with kernel code.

Possible this could be further simplified?

.... http://i1.kym-cdn.com/photos/images/facebook/000/234/739/fa5.jpg

BUG=https://github.com/dart-lang/sdk/issues/27394
R=sigmund@google.com, sra@google.com

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

[Emily Fortuna, 2016-11-18 18:41:34]

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