Track fields which are known to be always set to integer constants

Track fields which are known to be always set to integer constants

For each field which is accessed directly in the generated JavaScript
code track whether all the setters are setting the field to an integer
constant. If that is the case the codegen will always generate ===
comparison when comparing to an integer constant.

To know for sure that only integer constants are assigned the
functions having the getters need to be recompiled.

This gives ~10% on Delta Blue.

R=ngeoffray@google.com, floitsch@google.com

BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=8796

[Søren Gjesse, 2012-06-14 12:38:58]

I am not sure this is the right approach to achieve the desired result. The
information collected is specific to integer constants. However it should be
possible to extend it with other information related to the same type set on a
field.

Nicolas also mentioned that there was a better way to get the codegen to
generate the === comparison. The ssa graph pattern check in the codegen does not
look right.

[ngeoffray, 2012-06-14 13:01:21]

Initial comments

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1175: if (node.left is HFieldGet &&
node.right is HConstant) {
This is too late in the game. The node.builtin should be set for what you want
to optimize.

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1760: if (valueType.toString() ==
"int") {
You should change that to node.value.isInteger()

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/optimize.dart:476: 
Add a comment on why we're interested in recompiling the method.

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/optimize.dart:477: if (node.left is HFieldGet &&
node.right is HConstant) {
check node.right.isInteger()

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/optimize.dart:493: return node;
This code does not really belong here. I would create a separate pass that tries
to find things that would be really valuable that would deserve having the
method a recompilation candidate.

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/1/lib/compiler/implement...
lib/compiler/implementation/universe.dart:77:
fieldIntegerSetters.putIfAbsent(type.element, () => new Map<SourceString,
bool>());
line too long

[Søren Gjesse, 2012-06-15 13:19:59]

Addressed the comments from Nicolas. I also added tracking of the type set for
the fields in the initializer list.

I also added removal of the generated bailout function when recompiling a
function as it might not be needed any more, and it will be regenerated if
needed.

On Delta Blue this change now optimizes the following generated code:

function() {
  var t1 = this.direction;
  if (typeof t1 !== 'number') return this.isSatisfied$0$bailout(1, t1);
  return !(t1 === 1);
 }
function() {
  var t1 = this.direction;
  if (typeof t1 !== 'number') return this.output$0$bailout(1, t1);
  return t1 === 2 ? this.v2 : this.v1;
 }
function() {
  return $.eqB(this.direction, 2) ? this.v1 : this.v2;
 }

to this:

function() {
  return !(this.direction === 1);
 }
function() {
  return this.direction === 2 ? this.v2 : this.v1;
 }
function() {
  return this.direction === 2 ? this.v1 : this.v2;
 }

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/codegen.dart:1175: if (node.left is HFieldGet &&
node.right is HConstant) {
On 2012/06/14 13:01:21, ngeoffray wrote:
> This is too late in the game. The node.builtin should be set for what you want
> to optimize.

Removed all this as the SSA optimization phase now updates the guarenteedType of
the HFieldGet instruction causing the builtin === comparison to be generated.

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/codegen.dart:1760: if (valueType.toString() ==
"int") {
On 2012/06/14 13:01:21, ngeoffray wrote:
> You should change that to node.value.isInteger()

Done.

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/optimize.dart (right):

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/optimize.dart:476: 
On 2012/06/14 13:01:21, ngeoffray wrote:
> Add a comment on why we're interested in recompiling the method.

Done.

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/optimize.dart:477: if (node.left is HFieldGet &&
node.right is HConstant) {
On 2012/06/14 13:01:21, ngeoffray wrote:
> check node.right.isInteger()

Done.

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/optimize.dart:493: return node;
On 2012/06/14 13:01:21, ngeoffray wrote:
> This code does not really belong here. I would create a separate pass that
tries
> to find things that would be really valuable that would deserve having the
> method a recompilation candidate.

Moved all this to a new SSA pass.

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/un...
File lib/compiler/implementation/universe.dart (right):

http://codereview.chromium.org/10539156/diff/1/lib/compiler/implementation/un...
lib/compiler/implementation/universe.dart:77:
fieldIntegerSetters.putIfAbsent(type.element, () => new Map<SourceString,
bool>());
On 2012/06/14 13:01:21, ngeoffray wrote:
> line too long

Done.

[floitsch, 2012-06-15 15:00:50]

LGTM with comments.

I don't see where you handle the case of dynamic-setters.

class A {
  A() : t = 0;
  var t;
}

foo(x) => x.t = "str";

bar() {
  var a = new A();
  foo(a);
  a.t;  // <= should not be treated as int.
}

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:1197: Type type =
left.receiver.propagatedType.computeType(compiler);
move type into the 'if'?

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:1198: if (left.element != null &&
right.isInteger()) {
we know already that right is an integer.

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:127: if
(!fieldIntegerSetters.containsKey(type.element)) return false;
Why return false here?

[Søren Gjesse, 2012-06-18 11:32:17]

PTAL

You where absolutely right about the dynamic-setters. Changed the patch to
indicate a known type if there are no dynamic-setters either. If there is a
dynamic-setter make the JavaScript always generate a type guard, as having all
integer setters seems like a quite strong signal. Lets see if this blows up code
size.

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:1197: Type type =
left.receiver.propagatedType.computeType(compiler);
On 2012/06/15 15:00:50, floitsch wrote:
> move type into the 'if'?

Done.

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:1198: if (left.element != null &&
right.isInteger()) {
On 2012/06/15 15:00:50, floitsch wrote:
> we know already that right is an integer.

Done.

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/5001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:127: if
(!fieldIntegerSetters.containsKey(type.element)) return false;
On 2012/06/15 15:00:50, floitsch wrote:
> Why return false here?

Yes that is a good question. For the current use I should return true, as I
check that it is also initialized to an integer and therefore if there are no
setters it will stay an integer. Changed it to true.

[floitsch, 2012-06-18 13:20:40]

LGTM.

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:466: var oldCode =
world.universe.generatedCode[work.element];
Please type your code.

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
lib/compiler/implementation/ssa/optimize.dart:1220: // itselfalways sets an
integer in the fiels is still a strong
itself always

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
lib/compiler/implementation/universe.dart:90: bool
couldHaveFieldOnlyIntegerInitializer(Type type, SourceString name) {
just call hasFieldOnlyIntegerInitializer.
And shouldn't it be initializer*s* ?

[Søren Gjesse, 2012-06-19 14:35:00]

Forgot to post these comments yesterday.

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:466: var oldCode =
world.universe.generatedCode[work.element];
On 2012/06/18 13:20:40, floitsch wrote:
> Please type your code.

Done.

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
lib/compiler/implementation/ssa/optimize.dart:1220: // itselfalways sets an
integer in the fiels is still a strong
On 2012/06/18 13:20:40, floitsch wrote:
> itself always

Done.

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10539156/diff/15001/lib/compiler/imple...
lib/compiler/implementation/universe.dart:90: bool
couldHaveFieldOnlyIntegerInitializer(Type type, SourceString name) {
On 2012/06/18 13:20:40, floitsch wrote:
> just call hasFieldOnlyIntegerInitializer.
> And shouldn't it be initializer*s* ?

Done.