Compute KernelClosureRepresentationInfo.variableIsUsedInTryOrSync

Compute KernelClosureRepresentationInfo.variableIsUsedInTryOrSync

This handles one of the current inequivalences on hello world from dill.

R=efortuna@google.com

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

[Johnni Winther, 2017-06-15 14:29:39]

Description was changed from

==========
Compute KernelClosureRepresentationInfo.variableIsUsedInTryOrSync

This handles one of the currently inequivalences on hello world from dill.
==========

to

==========
Compute KernelClosureRepresentationInfo.variableIsUsedInTryOrSync

This handles one of the current inequivalences on hello world from dill.
==========

[Johnni Winther, 2017-06-15 14:30:59]

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

[Johnni Winther, 2017-06-15 14:31:00]

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

https://codereview.chromium.org/2938203003/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:583: registry.registerStaticUse(new
StaticUse.inlining(element));
Only register when we actually inline.

[Emily Fortuna, 2017-06-15 19:21:54]

lgtm.... though I am currently working on a change that connects with this too
(it changes the interface with how closure info is calculated):
https://codereview.chromium.org/2939163002

In my change I was going with a process where you calculate the info first and
then look up the cached stuff later. This version does an on-demand way. Is that
preferable for this situation?

[Emily Fortuna, 2017-06-15 19:21:55]

lgtm.... though I am currently working on a change that connects with this too
(it changes the interface with how closure info is calculated):
https://codereview.chromium.org/2939163002

In my change I was going with a process where you calculate the info first and
then look up the cached stuff later. This version does an on-demand way. Is that
preferable for this situation?

[Emily Fortuna, 2017-06-15 21:49:16]

lgtm

[Emily Fortuna, 2017-06-15 22:54:28]

https://codereview.chromium.org/2938203003/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/closure.dart (right):

https://codereview.chromium.org/2938203003/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/closure.dart:69: 
add @override?

[Johnni Winther, 2017-06-16 13:40:07]

https://codereview.chromium.org/2938203003/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/closure.dart (right):

https://codereview.chromium.org/2938203003/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/closure.dart:69: 
On 2017/06/15 22:54:28, Emily Fortuna wrote:
> add @override?

Done.

[Johnni Winther, 2017-06-16 14:34:51]

On 2017/06/16 13:40:07, Johnni Winther wrote:
>
https://codereview.chromium.org/2938203003/diff/20001/pkg/compiler/lib/src/ke...
> File pkg/compiler/lib/src/kernel/closure.dart (right):
> 
>
https://codereview.chromium.org/2938203003/diff/20001/pkg/compiler/lib/src/ke...
> pkg/compiler/lib/src/kernel/closure.dart:69: 
> On 2017/06/15 22:54:28, Emily Fortuna wrote:
> > add @override?
> 
> Done.

Don't wait for this to land. It's difficult to merge with
https://codereview.chromium.org/2939163002/ and no tests depend on it yet.

[Johnni Winther, 2017-06-19 12:51:56]

PTAL

[Emily Fortuna, 2017-06-19 17:44:41]

lgtm, but please see comments first

https://codereview.chromium.org/2938203003/diff/80001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2938203003/diff/80001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:230: closureDataLookup =
backendStrategy.createClosureConversionTask(this),
oh, do we not need this to be part of the task list? then we can change the API
on backend strategy to just call convertClosures directly rather than backend
strategy producing a task...

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/closure.dart:26: // TODO(johnniwinther,efortuna): Split
[ClosureConversionTask] from
don't worry. already working on that one. :-)

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/element_strategy.dart (left):

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/element_strategy.dart:47: ClosureConversionTask
createClosureConversionTask(Compiler compiler) =>
is taking this out the direction you want to go? I went the other way because I
thought I'd match the SsaBuilderTask, but then after Siggi pointed out you were
going the other direction:
https://codereview.chromium.org/2937203002/diff/40001/pkg/compiler/lib/src/ke...

I'm fine doing it this way -- I was just trying to be consistent! :-P

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_model/js_strategy.dart (right):

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_model/js_strategy.dart:40: KernelToElementMap
__elementMap;
not thrilled with the double underscore naming convention, but I don't have a
better suggestion. :-P

[Johnni Winther, 2017-06-20 10:33:12]

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/closure.dart:26: // TODO(johnniwinther,efortuna): Split
[ClosureConversionTask] from
On 2017/06/19 17:44:41, Emily Fortuna wrote:
> don't worry. already working on that one. :-)

Acknowledged.

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/element_strategy.dart (left):

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/element_strategy.dart:47: ClosureConversionTask
createClosureConversionTask(Compiler compiler) =>
On 2017/06/19 17:44:41, Emily Fortuna wrote:
> is taking this out the direction you want to go? I went the other way because
I
> thought I'd match the SsaBuilderTask, but then after Siggi pointed out you
were
> going the other direction:
>
https://codereview.chromium.org/2937203002/diff/40001/pkg/compiler/lib/src/ke...
> 
> I'm fine doing it this way -- I was just trying to be consistent! :-P

I want to get rid of CompilerTask as base classes. It is just a stop-watch and
should be passed to the actual task.

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_model/js_strategy.dart (right):

https://codereview.chromium.org/2938203003/diff/100001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_model/js_strategy.dart:40: KernelToElementMap
__elementMap;
On 2017/06/19 17:44:41, Emily Fortuna wrote:
> not thrilled with the double underscore naming convention, but I don't have a
> better suggestion. :-P

Acknowledged.

[Johnni Winther, 2017-06-20 10:45:57]

Description was changed from

==========
Compute KernelClosureRepresentationInfo.variableIsUsedInTryOrSync

This handles one of the current inequivalences on hello world from dill.
==========

to

==========
Compute KernelClosureRepresentationInfo.variableIsUsedInTryOrSync

This handles one of the current inequivalences on hello world from dill.

R=efortuna@google.com

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

[Johnni Winther, 2017-06-20 10:45:59]

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