dart2js cps: Replace GetLazyStatic with GetStatic.

dart2js cps: Replace GetLazyStatic with GetStatic.

Also fixes a bug in GVN that made it unable to GVN GetLazyStatic.

BUG=
R=sra@google.com

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

[asgerf, 2015-12-11 14:25:43]

asgerf@google.com changed reviewers:
+ kmillikin@google.com, sra@google.com

[sra1, 2015-12-11 17:13:09]

lgtm with types on map.

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart (right):

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:15: /// analysis in the
[GVN] pass, since [GetStatic] is known to have no effects.
"is known to have" -> "has"

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:20: final
Map<Continuation, Set<FieldElement>> loadedStaticsAt = {};
Put types on the map literal - that is what is checked.
(It is easy to accidentally add a Reference)

Perhaps use a HashMap instead of LinkedHashMap.

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:34: loadedStatics =
loadedStaticsAt[cont] ?? new Set<FieldElement>();
Why is it null?

I would add two helpers - newSet and cloneSet and put them side-by side so it is
easy to change the set type (e.g. HashSet instead of (LinkedHash)Set)

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:67: }
Could there already be a GetStatic?
Is the pass idempotent without adding it?

[asgerf, 2015-12-14 17:05:39]

PTAL at patch 5 -> 1 (contains a few merge changes, sorry).

I changed GetStatic to have a "witness" reference to the GetLazyStatic to
enforce that it does not get lifted above it.

This was not an observable problem because GetLazyStatic has all side effects,
but it would become an issue when that gets fixed.

There was also a bug in the IR builder that would translate compound operators
on lazily initialized static fields incorrectly.

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart (right):

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:15: /// analysis in the
[GVN] pass, since [GetStatic] is known to have no effects.
On 2015/12/11 17:13:09, sra1 wrote:
> "is known to have" -> "has"

Done.

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:20: final
Map<Continuation, Set<FieldElement>> loadedStaticsAt = {};
On 2015/12/11 17:13:08, sra1 wrote:
> Put types on the map literal - that is what is checked.
> (It is easy to accidentally add a Reference)
> 
> Perhaps use a HashMap instead of LinkedHashMap.

The pass has changed quite a bit now, but there are types on the maps and there
is a clone method so we can switch to non-linked in one place if we want.

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:34: loadedStatics =
loadedStaticsAt[cont] ?? new Set<FieldElement>();
On 2015/12/11 17:13:08, sra1 wrote:
> Why is it null?
> 
> I would add two helpers - newSet and cloneSet and put them side-by side so it
is
> easy to change the set type (e.g. HashSet instead of (LinkedHash)Set)

Null was for the case of unreachable continuations, but the pass uses scoping
now, so it's not needed anymore.

https://codereview.chromium.org/1521553003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:67: }
On 2015/12/11 17:13:08, sra1 wrote:
> Could there already be a GetStatic?
> Is the pass idempotent without adding it?

There can't be any GetStatics unless the pass is run twice, and in that case the
dominating GetLazyStatic/SetStatic should still be there.

GetStatic doesn't actually load the field, it just returns undefined if
uninitialized, so I didn't want to add it.

Since GetStatic has a witness reference now, I should be clear that the witness
is going to be in scope anyway, so it makes no difference.

[sra1, 2015-12-14 18:19:41]

lgtm

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1310: /// code motion.
Do we assert this condition (that witness is set)?
What is the witness if we add an analysis that determines the field is
initialized as a method precondition?

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1319: /// The value of the witness
is not used.
Do you think that all nodes should have witnesses as a general uniform safety
mechanism?

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/gvn.dart (right):

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/gvn.dart:108: //   confuse the LoopSideEffects
pre-analysis.
Can we remove this TODO?

[asgerf, 2015-12-15 10:29:56]

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1310: /// code motion.
On 2015/12/14 18:19:41, sra1 wrote:
> Do we assert this condition (that witness is set)?
> What is the witness if we add an analysis that determines the field is
> initialized as a method precondition?

We could add a "witness" parameter to the method and use that. The caller of
that method then needs to have a witness available at the call-site.

A far more pragmatic solution would be to mark the GetStatic as "pinned",
disabling all code motion, like HTypeKnown in the SSA.

Or if we know the current IR object will not be used for inlining in another
method, we could mark it as completely free to move around.

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1319: /// The value of the witness
is not used.
On 2015/12/14 18:19:41, sra1 wrote:
> Do you think that all nodes should have witnesses as a general uniform safety
> mechanism?

I prefer to use the "operand copy" instructions like Refinement, BoundsCheck,
and NullCheck when possible.  They scale better for primitives that need
multiple witnesses, e.g. NumAdd must know that both operands are numbers, and
having two extra witnesses just for that is too much.

We might need more explicit witness fields in time, but I think most uses can be
expressed by operand copy instructions.

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/gvn.dart (right):

https://codereview.chromium.org/1521553003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/gvn.dart:108: //   confuse the LoopSideEffects
pre-analysis.
On 2015/12/14 18:19:41, sra1 wrote:
> Can we remove this TODO?

Missed that, thanks.

[asgerf, 2015-12-15 10:46:03]

Description was changed from

==========
dart2js cps: Replace GetLazyStatic with GetStatic.

Also fixes a bug in GVN that made it unable to GVN GetLazyStatic.

BUG=
==========

to

==========
dart2js cps: Replace GetLazyStatic with GetStatic.

Also fixes a bug in GVN that made it unable to GVN GetLazyStatic.

BUG=
R=sra@google.com

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

[asgerf, 2015-12-15 10:46:04]

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