dart2js cps: Refactor tracking of side effects.

dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

BUG=
R=sigmund@google.com

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

Reverted: https://github.com/dart-lang/sdk/commit/a7fe3b140cf93f52cbed72d448c4229c4102ac6a

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

[asgerf, 2016-01-28 16:11:42]

Description was changed from

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

I plan to separate CodeMotion from GVN (again), and this should enable
that without duplicating the whole effect analysis yet again.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

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

to

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

I plan to separate CodeMotion from GVN (again), and this should enable
that without duplicating the whole effect analysis yet again.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

There are some loose ends in the CL which I plan to clean up next.
I suspect they will (slightly) affect generated code and it helps
to keep those separate from the refactoring.

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

[asgerf, 2016-01-28 16:17:09]

Description was changed from

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

I plan to separate CodeMotion from GVN (again), and this should enable
that without duplicating the whole effect analysis yet again.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

There are some loose ends in the CL which I plan to clean up next.
I suspect they will (slightly) affect generated code and it helps
to keep those separate from the refactoring.

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

to

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

I plan to separate CodeMotion from GVN (again), and this should enable
that without duplicating the whole effect analysis yet again.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Separate CodeMotion from GVN
- Simplify GvnVectorBuilder using dependency flags
- Stop the overuse of isSafeForElimination.

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

[asgerf, 2016-01-28 16:18:29]

Description was changed from

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

I plan to separate CodeMotion from GVN (again), and this should enable
that without duplicating the whole effect analysis yet again.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Separate CodeMotion from GVN
- Simplify GvnVectorBuilder using dependency flags
- Stop the overuse of isSafeForElimination.

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

to

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

I plan to separate CodeMotion from GVN (again), and this should enable
that without duplicating the whole effect analysis yet again.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Separate CodeMotion from GVN.
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

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

[asgerf, 2016-01-28 16:20:19]

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

[asgerf, 2016-02-17 16:12:36]

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

[asgerf, 2016-02-17 16:12:36]

Ping.

As I discussed with Kevin, the original motivation for this (separating code
motion and gvn) was mistaken, but I still think we need something like this.

If for nothing else, I still think it makes the code easier to understand.

So we may not want to commit this in its current form, but if you have any input
on the matter, please comment.

[Siggi Cherem (dart-lang), 2016-02-17 20:53:24]

lgtm

Just to make sure I saw all the differences. My main understanding is that
unlike SideEffects now:

a) there is no object wrapping the flags, you use the int directly where needed
(this in itself is very nice).

b) there is an explicit "other" area (but I didn't see it used yet - besides it,
are there other flags missing from SideEffects?)

c) there is a conversion between `changes <-> dependsOn` that was more
cumbersome to do before (although that should be fairly simple to add to
SideEffects if we want to)

d) it's integrated with EffectNumbers, so the vector you use for GVN is always
up to date with areas tracked by the effect flags

Did I miss something else?

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

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:248: int get effectFlags =>
Effects.none;
+ dartdoc

Also, low-priority but, what do you think about simply calling this `effects`?

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1296: bool isFinal = false;
+dartdoc

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart (right):

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:34: Expression
traverseLetPrim(LetPrim node) {
seems that this file change is unrelated to the rest. Did you mean to keep it
with this CL?

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/effects.dart (right):

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/effects.dart:31: static const int changesOther =
_changes << _other;
I didn't see the `other` area used anywhere in the different phases. What's the
purpose you have in mind for it?

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2303: node.isFinal =
isFinal(node.field);
since `isFinal(element)` is based only on information we have from earlier on,
could we simply initialize node.isFinal at creation time and make it a final
property?

(at least for GetField, GetStatic, and GetLazyStatic)?

[asgerf, 2016-02-29 12:48:17]

Your summary of the changes looks about right.

As for (b): the "other" flag is for completeness but is currently always used
together with one of the other bits.  It should be more useful when foreign code
annotations can use it, since external side effects and DOM manipulation could
be classified as "other" instead of "all side effects".

There is also a distinction between indexable length and indexable content,
which SideEffects does not have.

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

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:248: int get effectFlags =>
Effects.none;
On 2016/02/17 20:53:23, Siggi Cherem (dart-lang) wrote:
> + dartdoc
> 
> Also, low-priority but, what do you think about simply calling this `effects`?

Done.

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1296: bool isFinal = false;
On 2016/02/17 20:53:23, Siggi Cherem (dart-lang) wrote:
> +dartdoc

Done.

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart (right):

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/eagerly_load_statics.dart:34: Expression
traverseLetPrim(LetPrim node) {
On 2016/02/17 20:53:23, Siggi Cherem (dart-lang) wrote:
> seems that this file change is unrelated to the rest. Did you mean to keep it
> with this CL?

It's not unrelated.

It is meant to copy over of the new "isFinal" field when replacing GetLazyStatic
with a GetStatic.

But in case isFinal is true, it's pointless to create the GetStatic in the first
place, so I rewrote it to avoid the redundancy.

The traverseLetPrim method is overridden to handle the case where the visited
primitive is removed.

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/effects.dart (right):

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/effects.dart:31: static const int changesOther =
_changes << _other;
On 2016/02/17 20:53:23, Siggi Cherem (dart-lang) wrote:
> I didn't see the `other` area used anywhere in the different phases. What's
the
> purpose you have in mind for it?

It's just to ensure that these bits form a proper partition of all effects.

E.g. so "effects == 0" actually guarantees that there are no non-local side
effects.

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1645053002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2303: node.isFinal =
isFinal(node.field);
On 2016/02/17 20:53:24, Siggi Cherem (dart-lang) wrote:
> since `isFinal(element)` is based only on information we have from earlier on,
> could we simply initialize node.isFinal at creation time and make it a final
> property?
> 
> (at least for GetField, GetStatic, and GetLazyStatic)?

Yeah it's better that way. Done.

[asgerf, 2016-02-29 14:09:31]

Description was changed from

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

I plan to separate CodeMotion from GVN (again), and this should enable
that without duplicating the whole effect analysis yet again.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Separate CodeMotion from GVN.
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

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

to

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

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

[asgerf, 2016-02-29 14:10:12]

Description was changed from

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

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

to

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

BUG=
R=sigmund@google.com

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

[asgerf, 2016-02-29 14:10:14]

Committed patchset #8 (id:140001) manually as
7f6c587de6654ea0b33d23d1c55a8f2a29316d1f (presubmit successful).

[asgerf, 2016-02-29 14:30:53]

Description was changed from

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

BUG=
R=sigmund@google.com

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

to

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

BUG=
R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/a7fe3b140cf93f52cbed72d448c4229c4102ac6a
==========

[asgerf, 2016-02-29 14:41:08]

Description was changed from

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

BUG=
R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/a7fe3b140cf93f52cbed72d448c4229c4102ac6a
==========

to

==========
dart2js cps: Refactor tracking of side effects.

Primitives now getter for its side effects, as a bitmask.

This avoids a lot of code duplication between GVN, BoundsChecker, and
LoopSideEffects.

There are more flags than those in the SideEffects class (which is used
by the SSA backend, foreign code annotations, and type inference).

We are going to need these extra flags in general, but I want to use
them in the CPS first to see how they work out.

Upcoming changes which are "loose ends" in this CL to keep it manageable:
- Simplify GvnVectorBuilder using dependency flags.
- Stop the overuse of isSafeForElimination.
- Add the extra flags to the rest of the compiler.

BUG=
R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/a7fe3b140cf93f52cbed72d448c4229c4102ac6a

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

[asgerf, 2016-02-29 14:41:10]

Committed patchset #9 (id:160001) manually as
6f3b69d36519e1a0305b2879f9413b50662f87c4 (presubmit successful).

[sra1, 2016-02-29 23:40:34]

I'd like to see some of the operations be a little more abstract, e.g.

if (node.primitive.effects & Effects.changesIndexableLength != 0) ...

-->

if (Effects.hasChange(node.primitive.effects, Effects.changesIndexableLength))
...

https://codereview.chromium.org/1645053002/diff/160001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1645053002/diff/160001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1382: int get effects => isFinal ?
0 : Effects.dependsOnInstanceField;
isFinal ? Effects.none : Effects.dependsOnInstanceField

[asgerf, 2016-03-01 11:53:52]

On 2016/02/29 23:40:34, sra1 wrote:
> I'd like to see some of the operations be a little more abstract, e.g.
> 
> if (node.primitive.effects & Effects.changesIndexableLength != 0) ...
> 
> -->
> 
> if (Effects.hasChange(node.primitive.effects, Effects.changesIndexableLength))
> ...
> 
>
https://codereview.chromium.org/1645053002/diff/160001/pkg/compiler/lib/src/c...
> File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):
> 
>
https://codereview.chromium.org/1645053002/diff/160001/pkg/compiler/lib/src/c...
> pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1382: int get effects => isFinal
?
> 0 : Effects.dependsOnInstanceField;
> isFinal ? Effects.none : Effects.dependsOnInstanceField

Why do you want to hide the fact that we are working with bit masks?