Use .callMethod instead of LocalFunctionElement as key in inference

Use .callMethod instead of LocalFunctionElement as key in inference

This changes the inlining strategy a bit by solving a latent problem:
ClosedWorld.isCalledInLoop was computed using LocalFunctionElement but
queried using SynthesizedCallMethodX thus always returning false in
these cases.

R=sigmund@google.com

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

[Johnni Winther, 2017-06-30 16:37:50]

Description was changed from

==========
Use .callMethod instead of LocalFunctionElement as key in inference
==========

to

==========
Use .callMethod instead of LocalFunctionElement as key in inference

This changes the inlining strategy a bit by solving a latent problem:
ClosedWorld.isCalledInLoop was computed using LocalFunctionElement but
queried using SynthesizedCallMethodX thus always returning false in
these cases.
==========

[Johnni Winther, 2017-06-30 16:37:58]

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

[Siggi Cherem (dart-lang), 2017-06-30 18:13:54]

lgtm, with suggestions and questions

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/builder.dart (right):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/builder.dart:941: if
(element.functionDeclaration.isLocal) {
I was a bit confused by this extra check, but I think it is because some of the
invariants here are presented "out of order". The way it currently reads is like
"Because isLocal it can't be a new context, let me check that it is true with
the implicit invariant about how declarations are represented for local
functions."

An idea that might help here, would be to do it in the opposite order with some
helper variables that give name to the concepts ("here is the declaration for
local functions, let me make sure it can't be a new context in that case"). For
example:

FunctionElement declaration = element.functionDeclaration;
MethodElement declarationMethod = declaration is LocalFunctionElement
    ? declaration.callMethod
    : declaration;
bool needNewContext = declarationMethod != analyzedElement;

if (needNewContext) {
  assert(declarationMethod is ConstructorElement
      failedAt(element, 'unexpected ... '));
  visitor = new ElementGraphBuilder(declarationMethod, ...)
}

thoughts?

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/type_graph_dump.dart (left):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:156: if (element.isParameter)
{
I thought we still had parameter?

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/type_system.dart (right):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/type_system.dart:416: TypeInformation
allocateClosureForMethod(
allocateClosure (remove forMethod now?)

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/types.dart (right):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/types.dart:245:
_inferrer.inferrer.lookupDataOfMember(member.memberContext),
just checking - this is the same as before, just that member.memberContext is
also member for non-local functions?

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (left):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:1139: return
getMightBePassedToApply(element.expression);
neat to get rid of this, but this is unrelated to your change right? We simply
created everything earlier now, correct?

[Johnni Winther, 2017-06-30 19:02:02]

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/builder.dart (right):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/builder.dart:941: if
(element.functionDeclaration.isLocal) {
On 2017/06/30 18:13:54, Siggi Cherem (dart-lang) wrote:
> I was a bit confused by this extra check, but I think it is because some of
the
> invariants here are presented "out of order". The way it currently reads is
like
> "Because isLocal it can't be a new context, let me check that it is true with
> the implicit invariant about how declarations are represented for local
> functions."
> 
> An idea that might help here, would be to do it in the opposite order with
some
> helper variables that give name to the concepts ("here is the declaration for
> local functions, let me make sure it can't be a new context in that case").
For
> example:
> 
> FunctionElement declaration = element.functionDeclaration;
> MethodElement declarationMethod = declaration is LocalFunctionElement
>     ? declaration.callMethod
>     : declaration;
> bool needNewContext = declarationMethod != analyzedElement;
> 
> if (needNewContext) {
>   assert(declarationMethod is ConstructorElement
>       failedAt(element, 'unexpected ... '));
>   visitor = new ElementGraphBuilder(declarationMethod, ...)
> }
> 
> thoughts?

Copied in :)

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/type_graph_dump.dart (left):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:156: if (element.isParameter)
{
On 2017/06/30 18:13:54, Siggi Cherem (dart-lang) wrote:
> I thought we still had parameter?

Never used for the _GraphGenerator (see lines 65-81)

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/type_system.dart (right):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/type_system.dart:416: TypeInformation
allocateClosureForMethod(
On 2017/06/30 18:13:54, Siggi Cherem (dart-lang) wrote:
> allocateClosure (remove forMethod now?)

Done.

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/types.dart (right):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/types.dart:245:
_inferrer.inferrer.lookupDataOfMember(member.memberContext),
On 2017/06/30 18:13:54, Siggi Cherem (dart-lang) wrote:
> just checking - this is the same as before, just that member.memberContext is
> also member for non-local functions?

Yes, it's the same.

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (left):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:1139: return
getMightBePassedToApply(element.expression);
On 2017/06/30 18:13:54, Siggi Cherem (dart-lang) wrote:
> neat to get rid of this, but this is unrelated to your change right? We simply
> created everything earlier now, correct?

No, this was a fix for the same kind of discrepancy as with isCalledInLoop, only
Stephan had found this.

[Johnni Winther, 2017-06-30 19:07:44]

Description was changed from

==========
Use .callMethod instead of LocalFunctionElement as key in inference

This changes the inlining strategy a bit by solving a latent problem:
ClosedWorld.isCalledInLoop was computed using LocalFunctionElement but
queried using SynthesizedCallMethodX thus always returning false in
these cases.
==========

to

==========
Use .callMethod instead of LocalFunctionElement as key in inference

This changes the inlining strategy a bit by solving a latent problem:
ClosedWorld.isCalledInLoop was computed using LocalFunctionElement but
queried using SynthesizedCallMethodX thus always returning false in
these cases.

R=sigmund@google.com

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

[Johnni Winther, 2017-06-30 19:07:45]

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

[Siggi Cherem (dart-lang), 2017-06-30 20:34:36]

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (left):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:1139: return
getMightBePassedToApply(element.expression);
On 2017/06/30 19:02:02, Johnni Winther wrote:
> On 2017/06/30 18:13:54, Siggi Cherem (dart-lang) wrote:
> > neat to get rid of this, but this is unrelated to your change right? We
simply
> > created everything earlier now, correct?
> 
> No, this was a fix for the same kind of discrepancy as with isCalledInLoop,
only
> Stephan had found this.

I see - the comment above was misleading, it gave me the impression that some
elements were created after inference: they were created before but we were
using the wrong one

[Johnni Winther, 2017-07-03 07:26:07]

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (left):

https://codereview.chromium.org/2968743002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:1139: return
getMightBePassedToApply(element.expression);
On 2017/06/30 20:34:36, Siggi Cherem (dart-lang) wrote:
> On 2017/06/30 19:02:02, Johnni Winther wrote:
> > On 2017/06/30 18:13:54, Siggi Cherem (dart-lang) wrote:
> > > neat to get rid of this, but this is unrelated to your change right? We
> simply
> > > created everything earlier now, correct?
> > 
> > No, this was a fix for the same kind of discrepancy as with isCalledInLoop,
> only
> > Stephan had found this.
> 
> I see - the comment above was misleading, it gave me the impression that some
> elements were created after inference: they were created before but we were
> using the wrong one

When the comment was written they _were_ created after inference :)