Set runtime type information when converting list literals.

Set runtime type information when converting list literals.

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

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

[Emily Fortuna, 2016-11-15 19:36:31]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2016-11-15 19:38:25]

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

[Harry Terkelsen, 2016-11-15 22:44:48]

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

https://codereview.chromium.org/2502033002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:211: void
pushInvokeStatic(MethodElement element, List<HInstruction> arguments,
This class should not have any methods with Element or Node arguments. The
reason for KernelAstAdapter is so that this class can only ever deal with Kernel
nodes, and not the dart2js Element model. It looks like you need
pushInvokeStatic in order to use it from GraphBuilder. But the GraphBuilder only
needs to make one specific invocation (to backend.helpers.setRuntimeTypeInfo).
Maybe the method in GraphBuilder should be 'setRuntimeTypeInfo(...)' which uses
_pushStaticInvocation in this class and pushInvokeStatic in the normal builder

https://codereview.chromium.org/2502033002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:227: // TODO(5346): Try to avoid
the need for calling [declaration] before
this TODO makes no sense to me?

[Emily Fortuna, 2016-11-16 01:53:45]

okay, let's try this again!

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

https://codereview.chromium.org/2502033002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:211: void
pushInvokeStatic(MethodElement element, List<HInstruction> arguments,
On 2016/11/15 22:44:48, Harry Terkelsen wrote:
> This class should not have any methods with Element or Node arguments. The
> reason for KernelAstAdapter is so that this class can only ever deal with
Kernel
> nodes, and not the dart2js Element model. It looks like you need
> pushInvokeStatic in order to use it from GraphBuilder. But the GraphBuilder
only
> needs to make one specific invocation (to backend.helpers.setRuntimeTypeInfo).
> Maybe the method in GraphBuilder should be 'setRuntimeTypeInfo(...)' which
uses
> _pushStaticInvocation in this class and pushInvokeStatic in the normal builder

Thank you, I intended to get rid of this, but got distracted. Should be fixed
now!

https://codereview.chromium.org/2502033002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:227: // TODO(5346): Try to avoid
the need for calling [declaration] before
On 2016/11/15 22:44:48, Harry Terkelsen wrote:
> this TODO makes no sense to me?

Agreed.... it was copied from builder.dart. Is it better to leave it out? I
don't know what it means, but I thought I'd leave it in case the person who does
could see it. At any rate, this method is gone from here, so it's not really a
problem anymore. :-P

[Emily Fortuna, 2016-11-16 01:53:57]

Patchset #2 (id:40001) has been deleted

[Harry Terkelsen, 2016-11-16 17:26:47]

lgtm

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

https://codereview.chromium.org/2502033002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:594: listInstruction =
setRtiIfNeeded(listInstruction, listLiteral);
should we 'add' it after we set rti?

[Emily Fortuna, 2016-11-16 18:53:11]

Description was changed from

==========
Set runtime type information when converting list literals.

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

to

==========
Set runtime type information when converting list literals.

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

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

[Emily Fortuna, 2016-11-16 18:53:12]

Committed patchset #2 (id:60001) manually as
226e47a1273e79cfaf1d771048380cff5bbf63d5 (presubmit successful).

[Emily Fortuna, 2016-11-16 18:53:23]

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

https://codereview.chromium.org/2502033002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:594: listInstruction =
setRtiIfNeeded(listInstruction, listLiteral);
On 2016/11/16 17:26:47, Harry Terkelsen wrote:
> should we 'add' it after we set rti?

I talked to Stephen. The answer is no; we need to add this instruction to the
basic block before we add other instructions like runtime type info.