Collect getters in a FunctionSet while resolving.

Collect getters in a FunctionSet while resolving.
Committed: https://code.google.com/p/dart/source/detail?r=12184

[ahe, 2012-09-10 15:15:21]

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/world.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/world.dart:8: final FunctionSet getters;
Could you add a method named something like "recordUserDefinedGetter" instead of
using [getters] directly.

[ahe, 2012-09-10 17:24:20]

compiler and enqueuer, LGTM

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2027:
compiler.world.getters.hasOneElementMatchingSelector(selector);
Too many dots.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2027:
compiler.world.getters.hasOneElementMatchingSelector(selector);
I assume you mean "exactly one"? If so, perhaps adjust the method name.

[kasperl, 2012-09-11 07:39:11]

LGTM.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:105:
compiler.world.getters.add(element);
I think this deserves its own method in world -- that will also make it easier
to document that you're only collecting proper user-defined getters, not getters
implicitly introduced by fields.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1308: final bool
possibleTargetsAreFields;
Maybe this should be allPossibleTargetsAreFields -- or maybe it would be cleaner
with just having the field called hasSideEffects?

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1376: final bool _hasSideEffects;
Is this private just because of naming conflicts? Could this be called
mayHaveSideEffects? Or turn it around and call it isSideEffectFree? That name
would work will for HInvokeDynamicGetter too.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/universe/function_set.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/universe/function_set.dart:50: * Returns whether the
set has an element matching this selector.
has any elements matching the given [selector].

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/universe/function_set.dart:52: bool
hasOneElementMatchingSelector(Selector selector) {
hasAnyElementsMatchingSelector would be more correct.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/world.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/world.dart:8: final FunctionSet getters;
Maybe change name to userDefinedGetters or nonTrivialGetters?

[Søren Gjesse, 2012-09-11 08:06:01]

lgtm

https://chromiumcodereview.appspot.com/10911181/diff/2001/tests/compiler/dart...
File tests/compiler/dart2js/gvn_dynamic_field_get_test.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/tests/compiler/dart...
tests/compiler/dart2js/gvn_dynamic_field_get_test.dart:26:
checkNumberOfMatches(matches, 1);
Shouldn't this test also contain a lookup of the getter "foo" in the World
getters function set?

[ngeoffray, 2012-09-11 08:51:08]

Thanks for all the comments! PTAL to make sure you agree with the new function
names.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:105:
compiler.world.getters.add(element);
On 2012/09/11 07:39:11, kasperl wrote:
> I think this deserves its own method in world -- that will also make it easier
> to document that you're only collecting proper user-defined getters, not
getters
> implicitly introduced by fields.

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2027:
compiler.world.getters.hasOneElementMatchingSelector(selector);
On 2012/09/10 17:24:20, ahe wrote:
> Too many dots.

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2027:
compiler.world.getters.hasOneElementMatchingSelector(selector);
On 2012/09/10 17:24:20, ahe wrote:
> I assume you mean "exactly one"? If so, perhaps adjust the method name.

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1308: final bool
possibleTargetsAreFields;
On 2012/09/11 07:39:11, kasperl wrote:
> Maybe this should be allPossibleTargetsAreFields -- or maybe it would be
cleaner
> with just having the field called hasSideEffects?

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1376: final bool _hasSideEffects;
On 2012/09/11 07:39:11, kasperl wrote:
> Is this private just because of naming conflicts? Could this be called
> mayHaveSideEffects? Or turn it around and call it isSideEffectFree? That name
> would work will for HInvokeDynamicGetter too.

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/universe/function_set.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/universe/function_set.dart:50: * Returns whether the
set has an element matching this selector.
On 2012/09/11 07:39:11, kasperl wrote:
> has any elements matching the given [selector].

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/universe/function_set.dart:52: bool
hasOneElementMatchingSelector(Selector selector) {
On 2012/09/11 07:39:11, kasperl wrote:
> hasAnyElementsMatchingSelector would be more correct.

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/world.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/world.dart:8: final FunctionSet getters;
On 2012/09/10 15:15:21, ahe wrote:
> Could you add a method named something like "recordUserDefinedGetter" instead
of
> using [getters] directly.

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/lib/compiler/implem...
lib/compiler/implementation/world.dart:8: final FunctionSet getters;
On 2012/09/11 07:39:11, kasperl wrote:
> Maybe change name to userDefinedGetters or nonTrivialGetters?

Done.

https://chromiumcodereview.appspot.com/10911181/diff/2001/tests/compiler/dart...
File tests/compiler/dart2js/gvn_dynamic_field_get_test.dart (right):

https://chromiumcodereview.appspot.com/10911181/diff/2001/tests/compiler/dart...
tests/compiler/dart2js/gvn_dynamic_field_get_test.dart:26:
checkNumberOfMatches(matches, 1);
On 2012/09/11 08:06:01, Søren Gjesse wrote:
> Shouldn't this test also contain a lookup of the getter "foo" in the World
> getters function set?

Done.

[kasperl, 2012-09-11 11:13:48]

LGTM.