Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10964016:
Change the type inference for fields in dart2js (Closed)
|
Created:
8 years, 3 months ago by Søren Gjesse Modified:
8 years, 2 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionChange the type inference for fields in dart2js
The type inference for fields now uses the same mechanism as is
used for type inference for arguments and return types. That
means that compilation is optimistically taking the current
inferred type for a field. When an inferred type for a field
changes all functions previously compiled under the previous type
assumption are scheduled for recompilation.
There is special handling of constructors where fields which are
always set in a constructor is identified. This is done to avoid
having to combine with the null type which can be set by the
initializer list.
This change removes the two phase compilation previously used for
inferring field types.
R=kasperl@google.com, ngeoffray@google.com
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=12956
Patch Set 1 #Patch Set 2 : Minor fixes and rebased #
Total comments: 36
Patch Set 3 : Addressed revew comments from negoffray@ #Patch Set 4 : Addressed review comments from ngeoffray@ #Patch Set 5 : Rebased to r12709 #
Total comments: 26
Patch Set 6 : Rebased to r12757 #
Total comments: 2
Patch Set 7 : Addressed second round of comments #
Total comments: 17
Patch Set 8 : Addressed third round of comments and added more tests #Patch Set 9 : Rebased to r12841 #
Total comments: 26
Patch Set 10 : Addressed more comments and added more tests #
Total comments: 43
Patch Set 11 : Final round of changes #Messages
Total messages: 13 (0 generated)
http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... File lib/compiler/implementation/enqueue.dart (right): http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/enqueue.dart:263: compiler.backend.addedDynamicSetter(selector); I don't understand why you have to do that here. It looks like the codegen could just do it. http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... File lib/compiler/implementation/js_backend/backend.dart (right): http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/js_backend/backend.dart:230: final Map<Element, HType> fieldTypeMap; Why do you need three maps and not one? http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/js_backend/backend.dart:285: setterType == null) { Not sure what all these checks really mean. If the type is unknown shouldn't you invalidate the type in the maps? http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/js_backend/backend.dart:292: optimizedFunctions.getKeys().forEach((Element field) { I would do optimizedFunctions.forEach((Element field, _) { ...}); http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/js_backend/backend.dart:318: void registerOptimization(FunctionElement element, registerOptimization -> registerOptimizedFunction http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/js_backend/backend.dart:322: optimizedFunctions.putIfAbsent( FunctionSet set = optimizedFunctions... set.add(element) http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... File lib/compiler/implementation/ssa/optimize.dart (right): http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1217: // type after construction. This is mainly done to override to possible null to override to -> to override. But the sentence is still hard to parse. http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1248: currentBlock = node; Do you need currentBlock? http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1257: // all of them. I don't really like these null checks here. Could you have the map of a block be a copy the map of its first predecessor (or a new Map for the first block), and then intersect it with the maps of the previous predecessors? http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1281: HInstruction instruction = node.first; Change this loop to block.forEachInstruction(..._ http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1290: exitDominatedSetters[field] = type; Do you need this? I would think that when you reach the exit block, the intersection of its predecessors would contain the list of fields that have been initialized. http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1303: visitForeignNew(HForeignNew node) { Document which element has a foreign new, and what it means to go through its arguments. http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1329: if (work.element.isGenerativeConstructorBody() && Why this first check? Otherwise it's a generative constructor and it does not have a body. http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1330: node.element.isMember() && Shouldn't you also check that the class of the element is the current class? http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1332: node.block.dominates(currentGraph.exit)) { Again, I don't think this is needed if you just look at the fieldSetters of the exit block. http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1334: } else { I don't understand why an else here. http://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/optimize.dart:1341: currentFieldSetters[field] = currentFieldSetters[field].union(type); Maybe fetch currentFieldSetters[field] before, and check for null. http://codereview.chromium.org/10964016/diff/2001/tests/compiler/dart2js/meta... File tests/compiler/dart2js/metadata_test.dart (right): http://codereview.chromium.org/10964016/diff/2001/tests/compiler/dart2js/meta... tests/compiler/dart2js/metadata_test.dart:25: compiler.enqueuer.resolution.queueIsClosed = false; Looks fishy to do this here.
PTAL I changed the SSA propagation of fields set in the generative constructor body. I is now much cleaner. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/enqueue.dart (right): https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:263: compiler.backend.addedDynamicSetter(selector); On 2012/09/20 14:43:01, ngeoffray wrote: > I don't understand why you have to do that here. It looks like the codegen could > just do it. Having it here ensures that we get just one call when a dynamic setter is added. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:230: final Map<Element, HType> fieldTypeMap; On 2012/09/20 14:43:01, ngeoffray wrote: > Why do you need three maps and not one? I need to separate the types set by the initializers and the types set by the constructors to completely override the initializer type if a type is set in the constructor. This is to avoid the XXX_OR_NULL type if the initializer just initialize to null. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:285: setterType == null) { On 2012/09/20 14:43:01, ngeoffray wrote: > Not sure what all these checks really mean. If the type is unknown shouldn't you > invalidate the type in the maps? This checks whether there is any type information for the field. If not then we don't register the type as having no information leads to type UNKNOWN. If type information is revealed later i the compilation process we will start registering it. Added comment. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:292: optimizedFunctions.getKeys().forEach((Element field) { On 2012/09/20 14:43:01, ngeoffray wrote: > I would do optimizedFunctions.forEach((Element field, _) { ...}); Done. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:318: void registerOptimization(FunctionElement element, On 2012/09/20 14:43:01, ngeoffray wrote: > registerOptimization -> registerOptimizedFunction Done. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:322: optimizedFunctions.putIfAbsent( On 2012/09/20 14:43:01, ngeoffray wrote: > FunctionSet set = optimizedFunctions... > set.add(element) This pattern is used in many places. Is it the performance penalty by making two lookups in the Map you don't like? https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/optimize.dart (right): https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1217: // type after construction. This is mainly done to override to possible null On 2012/09/20 14:43:01, ngeoffray wrote: > to override to -> to override. > > But the sentence is still hard to parse. Rephrased. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1248: currentBlock = node; On 2012/09/20 14:43:01, ngeoffray wrote: > Do you need currentBlock? No. It is an instance variable in the HBaseVisitor base class though. Removed the line. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1257: // all of them. On 2012/09/20 14:43:01, ngeoffray wrote: > I don't really like these null checks here. Could you have the map of a block be > a copy the map of its first predecessor (or a new Map for the first block), and > then intersect it with the maps of the previous predecessors? Done. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1281: HInstruction instruction = node.first; On 2012/09/20 14:43:01, ngeoffray wrote: > Change this loop to block.forEachInstruction(..._ Done. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1290: exitDominatedSetters[field] = type; On 2012/09/20 14:43:01, ngeoffray wrote: > Do you need this? I would think that when you reach the exit block, the > intersection of its predecessors would contain the list of fields that have been > initialized. The exitDominatedSetters is now gone. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1303: visitForeignNew(HForeignNew node) { On 2012/09/20 14:43:01, ngeoffray wrote: > Document which element has a foreign new, and what it means to go through its > arguments. Done. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1329: if (work.element.isGenerativeConstructorBody() && On 2012/09/20 14:43:01, ngeoffray wrote: > Why this first check? Otherwise it's a generative constructor and it does not > have a body. Removed. I was not 100% sure of what the generative constructor could contain. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1330: node.element.isMember() && On 2012/09/20 14:43:01, ngeoffray wrote: > Shouldn't you also check that the class of the element is the current class? Done. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1332: node.block.dominates(currentGraph.exit)) { On 2012/09/20 14:43:01, ngeoffray wrote: > Again, I don't think this is needed if you just look at the fieldSetters of the > exit block. Done. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1334: } else { On 2012/09/20 14:43:01, ngeoffray wrote: > I don't understand why an else here. The code here is now simplified. https://codereview.chromium.org/10964016/diff/2001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1341: currentFieldSetters[field] = currentFieldSetters[field].union(type); On 2012/09/20 14:43:01, ngeoffray wrote: > Maybe fetch currentFieldSetters[field] before, and check for null. Done. https://codereview.chromium.org/10964016/diff/2001/tests/compiler/dart2js/met... File tests/compiler/dart2js/metadata_test.dart (right): https://codereview.chromium.org/10964016/diff/2001/tests/compiler/dart2js/met... tests/compiler/dart2js/metadata_test.dart:25: compiler.enqueuer.resolution.queueIsClosed = false; On 2012/09/20 14:43:01, ngeoffray wrote: > Looks fishy to do this here. Agree - it is a side effect of removing the recompilation queue. I will look more into it.
https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... File lib/compiler/implementation/js_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/js_backend/backend.dart:296: if (compiler.codegenWorld.hasInvokedSetter(field, compiler)) { Why do you need the hasInvokedSetter check? The selector tells that there is one, right? https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/js_backend/backend.dart:297: scheduleRecompilation(field); So you invalidate even though the type may not have changed, right? Is this to handle 'final'? Should you have another set of opimized functions for this? https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/js_backend/backend.dart:306: if (compiler.codegenWorld.hasInvokedSetter(field, compiler)) { Isn't this too pessimistic? Maybe you should mark the type as conflict internally, meaning the FieldTypesRegistry decided to just invalidate optimizations on this field. Looks better because then deciding to optimize remains internal. https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... File lib/compiler/implementation/ssa/codegen.dart (right): https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/codegen.dart:1643: if (!work.element.isGenerativeConstructorBody()) { Please add a comment on why you're checking this. https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/codegen.dart:1646: backend.registerFieldSetter(node, types); Does the backend have to know about types/nodes? You couls just pass element/type, right? https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... File lib/compiler/implementation/ssa/optimize.dart (right): https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/optimize.dart:50: new SsaConstructionFieldTypes(backend, work, types)]; This will also be run by the speculative optimization. Please fix it or otherwise we could start optimizing on speculative types (I think you check for it in the optimization phase, but it's very error prone). https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/optimize.dart:616: if (work.element.isMember()) { Why this check? https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/optimize.dart:1250: // Build a map which unions the fields from all predecessors. It's a union of the types but an intersection of the fields. https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/optimize.dart:1253: // Loop headers are the only nodes with back edges. Please also assert that they only have 2 predecessors. https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/optimize.dart:1278: // Instruction not explicitly handled keeps the current inferred types. Kasper suggested to do the other way around and have a white-list of instructions instead, and I agree it feels safer. So I'd prefer if you'd check that instruction uses 'this' and mark thisExposed if yes. You could construct a set of all users of this before visiting the graph, and the check would be cheap. https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/optimize.dart:1304: assert(field.isMember()); Why this assert? https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/ssa/optimize.dart:1325: backend.registerFieldConstructor(element, HType.UNKNOWN); Why do you have to say they're unknown? https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... File lib/compiler/implementation/universe/partial_type_tree.dart (right): https://chromiumcodereview.appspot.com/10964016/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/partial_type_tree.dart:5: class PartialTypeTree implements Hashable { Do you need this change in this CL? https://chromiumcodereview.appspot.com/10964016/diff/14003/lib/compiler/imple... File lib/compiler/implementation/enqueue.dart (right): https://chromiumcodereview.appspot.com/10964016/diff/14003/lib/compiler/imple... lib/compiler/implementation/enqueue.dart:293: if (compiler.backend is js_backend.JavaScriptBackend) { So this has two effects: cancel previous optimizations and avoid doing new optimizations based on the field. It looks like you should check for setter kind here on the selector, otherwise you would invalidate even if it's a method that is being called. Also, I really don't like having this here. You're loosing the type information on the selector, which might invalidate or not the optimizations.
ptal http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... File lib/compiler/implementation/js_backend/backend.dart (right): http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:296: if (compiler.codegenWorld.hasInvokedSetter(field, compiler)) { On 2012/09/24 08:15:49, ngeoffray wrote: > Why do you need the hasInvokedSetter check? The selector tells that there is > one, right? Changed this to have a local selector collection. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:297: scheduleRecompilation(field); On 2012/09/24 08:15:49, ngeoffray wrote: > So you invalidate even though the type may not have changed, right? Is this to > handle 'final'? Should you have another set of opimized functions for this? I will postpone taking the type of the dynamic setter into account to a follow-up CL. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:306: if (compiler.codegenWorld.hasInvokedSetter(field, compiler)) { On 2012/09/24 08:15:49, ngeoffray wrote: > Isn't this too pessimistic? Maybe you should mark the type as conflict > internally, meaning the FieldTypesRegistry decided to just invalidate > optimizations on this field. Looks better because then deciding to optimize > remains internal. See comments above. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... File lib/compiler/implementation/ssa/codegen.dart (right): http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:1643: if (!work.element.isGenerativeConstructorBody()) { On 2012/09/24 08:15:49, ngeoffray wrote: > Please add a comment on why you're checking this. Done. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:1646: backend.registerFieldSetter(node, types); On 2012/09/24 08:15:49, ngeoffray wrote: > Does the backend have to know about types/nodes? You couls just pass > element/type, right? No. Changed to pass Element and HType. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... File lib/compiler/implementation/ssa/optimize.dart (right): http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:50: new SsaConstructionFieldTypes(backend, work, types)]; On 2012/09/24 08:15:49, ngeoffray wrote: > This will also be run by the speculative optimization. Please fix it or > otherwise we could start optimizing on speculative types (I think you check for > it in the optimization phase, but it's very error prone). Done. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:616: if (work.element.isMember()) { On 2012/09/24 08:15:49, ngeoffray wrote: > Why this check? Because I did not add support for static or top level functions in the field types registry. This has now been added and this check removed. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1250: // Build a map which unions the fields from all predecessors. On 2012/09/24 08:15:49, ngeoffray wrote: > It's a union of the types but an intersection of the fields. Updated comments. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1253: // Loop headers are the only nodes with back edges. On 2012/09/24 08:15:49, ngeoffray wrote: > Please also assert that they only have 2 predecessors. Done. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1278: // Instruction not explicitly handled keeps the current inferred types. On 2012/09/24 08:15:49, ngeoffray wrote: > Kasper suggested to do the other way around and have a white-list of > instructions instead, and I agree it feels safer. So I'd prefer if you'd check > that instruction uses 'this' and mark thisExposed if yes. You could construct a > set of all users of this before visiting the graph, and the check would be > cheap. Done. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1304: assert(field.isMember()); On 2012/09/24 08:15:49, ngeoffray wrote: > Why this assert? Good question - removed. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1325: backend.registerFieldConstructor(element, HType.UNKNOWN); On 2012/09/24 08:15:49, ngeoffray wrote: > Why do you have to say they're unknown? Because I just know that these fields have been set to something somewhere in the constructor, but not on all control patch. Ideally this should be unioned with the field initializer type for this field, but then we should have a flag here to indicate that the field might be set in the constructor together with the type. http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... File lib/compiler/implementation/universe/partial_type_tree.dart (right): http://codereview.chromium.org/10964016/diff/17002/lib/compiler/implementatio... lib/compiler/implementation/universe/partial_type_tree.dart:5: class PartialTypeTree implements Hashable { On 2012/09/24 08:15:49, ngeoffray wrote: > Do you need this change in this CL? Not anymore. Thanks for noticing. http://codereview.chromium.org/10964016/diff/14003/lib/compiler/implementatio... File lib/compiler/implementation/enqueue.dart (right): http://codereview.chromium.org/10964016/diff/14003/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:293: if (compiler.backend is js_backend.JavaScriptBackend) { On 2012/09/24 08:15:49, ngeoffray wrote: > So this has two effects: cancel previous optimizations and avoid doing new > optimizations based on the field. > > It looks like you should check for setter kind here on the selector, otherwise > you would invalidate even if it's a method that is being called. > > Also, I really don't like having this here. You're loosing the type information > on the selector, which might invalidate or not the optimizations. Moved to codegen.
LGTM, with a few comments http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... File lib/compiler/implementation/js_backend/backend.dart (right): http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:251: optimizedStaticFunctions.remove(field); Put the remove in the if? http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:256: optimizedFunctions.remove(field); ditto http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:295: if (selectorSet.contains(setter.name)) return; Please add comments on the field saying that this set contains the fields that cannot be optimized. Maybe also find a better name for it? fieldThatCannotBeUsedForOptimization work for me. http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:322: if (type !== null) result = result.union(type); You could avoid these checks and union if you compute the information after visiting the constructor and constructor body. Thinking about it, why couldn't you have just one map from element to type, that gets updated the further you go in the analysis? (First the initializer, then the constructor, then the field set). http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/codegen.dart (right): http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:1645: // ssa optimizer. Missing verb in the sentence. http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/optimize.dart (right): http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:79: new SsaConstructionFieldTypes(backend, work, types)]; That's not right, the speculative optimization should not run this phase. My comment was that this phase should be run only once, and since you add it to the list line 50, then it will be run twice (normal optimizer and speculative optimizer share the same list). http://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementatio... lib/compiler/implementation/ssa/optimize.dart:1334: backend.registerFieldConstructor(element, HType.UNKNOWN); So this is just to prevent thinking a field has this type because of an initializer? Please add a comment. http://codereview.chromium.org/10964016/diff/18001/tests/compiler/dart2js/fie... File tests/compiler/dart2js/field_type_inferer_test.dart (right): http://codereview.chromium.org/10964016/diff/18001/tests/compiler/dart2js/fie... tests/compiler/dart2js/field_type_inferer_test.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file We also discussed about tests for subclasses and multiple constructors. Did you forget to svn add it?
Please look once more Addressed review comments. Also added tests and it turned out that when multiple constructors are present the field type inference has to be turned off. This can be improved by keeping track of the generative constructor/generative constructor body pairs. Turns out that there are some test failures in checked mode. I am looking into these. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... File lib/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:251: optimizedStaticFunctions.remove(field); On 2012/09/24 15:39:29, ngeoffray wrote: > Put the remove in the if? Done. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:256: optimizedFunctions.remove(field); On 2012/09/24 15:39:29, ngeoffray wrote: > ditto Done. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:295: if (selectorSet.contains(setter.name)) return; On 2012/09/24 15:39:29, ngeoffray wrote: > Please add comments on the field saying that this set contains the fields that > cannot be optimized. Maybe also find a better name for it? > fieldThatCannotBeUsedForOptimization work for me. Changed name to setterSelectorsUsed and added comment. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:322: if (type !== null) result = result.union(type); On 2012/09/24 15:39:29, ngeoffray wrote: > You could avoid these checks and union if you compute the information after > visiting the constructor and constructor body. Thinking about it, why couldn't > you have just one map from element to type, that gets updated the further you go > in the analysis? (First the initializer, then the constructor, then the field > set). As far as I can see the generative constructor, generative constructor body and methods/functions using member fields can be compiled in any order. Therefore it seems quite difficult to avoid making this computation each time. I could keep the final result in an additional map so the computation will only have to be done when new information arrives. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... File lib/compiler/implementation/ssa/codegen.dart (right): https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/ssa/codegen.dart:1645: // ssa optimizer. On 2012/09/24 15:39:29, ngeoffray wrote: > Missing verb in the sentence. Done. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... File lib/compiler/implementation/ssa/optimize.dart (right): https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:79: new SsaConstructionFieldTypes(backend, work, types)]; On 2012/09/24 15:39:29, ngeoffray wrote: > That's not right, the speculative optimization should not run this phase. My > comment was that this phase should be run only once, and since you add it to the > list line 50, then it will be run twice (normal optimizer and speculative > optimizer share the same list). Sorry my bad. As discussed offline added boolean argument to optimize.'to indicate whether this is the speculative phase. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1334: backend.registerFieldConstructor(element, HType.UNKNOWN); On 2012/09/24 15:39:29, ngeoffray wrote: > So this is just to prevent thinking a field has this type because of an > initializer? Please add a comment. Done. https://codereview.chromium.org/10964016/diff/18001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1334: backend.registerFieldConstructor(element, HType.UNKNOWN); On 2012/09/24 15:39:29, ngeoffray wrote: > So this is just to prevent thinking a field has this type because of an > initializer? Please add a comment. Done.
Missed a single comment. https://chromiumcodereview.appspot.com/10964016/diff/18001/tests/compiler/dar... File tests/compiler/dart2js/field_type_inferer_test.dart (right): https://chromiumcodereview.appspot.com/10964016/diff/18001/tests/compiler/dar... tests/compiler/dart2js/field_type_inferer_test.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file On 2012/09/24 15:39:29, ngeoffray wrote: > We also discussed about tests for subclasses and multiple constructors. Did you > forget to svn add it? Added more tests. Turned out that for classes several constructors I need to turn off field type optimization as it is currently implemented. TEST_15 reveals that.
Sorry, still have a few more comments :) https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... File lib/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:228: final Map<Element, Set<Element>> constructors; Please explain why we have three maps and that in theory we could just have one. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:292: if (ctors.contains(element)) return; Looks like this will never happen right? Maybe assert the negation instead? https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:294: if (ctors.length == 2) { Please add a comment and a TODO. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:315: if (type == HType.UNKNOWN && weird identation. I believe the style is more: a && foo && ... Or: a && foo && ... I prefer the latter. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... File lib/compiler/implementation/ssa/codegen.dart (right): https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/codegen.dart:1645: // separate step in the ssa optimizer. Please name the step. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/codegen.dart:1649: backend.registerFieldSetter(work.element, node.element, types[node.value]); Line too long https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... File lib/compiler/implementation/ssa/optimize.dart (right): https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:29: void optimize(WorkItem work, HGraph graph, bool speculative) { I see no change of the callers of this method. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1270: newFieldSetters[element] = type; Why are you setting it here? if currentFieldSetters doesn't have it, it means it hasn't initialized it, right? So you should not add it to the newFieldSetters. Hmmm, if I'm right, maybe you got lucky with the tests. Adding a new one might be helpful. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1280: (HInstruction instruction) => instruction.accept(this)); You need to visit the phis, since they may use 'this'. Might need another test too :) https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1337: // For other fields having setters in the generative constructor body set body set -> body, set https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... File tests/compiler/dart2js/field_type_inferer_test.dart (right): https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:203: A() { fs = 1; } Is that turned into a dynamic setter? If not, I can't find the code in your change that prevents fs to be typed 'int'. https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:279: runTest(TEST_8, {'f': HType.UNKNOWN}); unindent TEST_8 https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:284: runTest(TEST_13, {'f': HType.UNKNOWN}); Add TODO(sgjesse): we should actually infer int.
Ready for another round. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... File lib/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:228: final Map<Element, Set<Element>> constructors; On 2012/09/25 21:15:43, ngeoffray wrote: > Please explain why we have three maps and that in theory we could just have one. Done. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:292: if (ctors.contains(element)) return; On 2012/09/25 21:15:43, ngeoffray wrote: > Looks like this will never happen right? Maybe assert the negation instead? It can, as the same constructor can be compiled more than once if some of the optimistic assumptions made when compiling are invalidated. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:294: if (ctors.length == 2) { On 2012/09/25 21:15:43, ngeoffray wrote: > Please add a comment and a TODO. Done. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:315: if (type == HType.UNKNOWN && On 2012/09/25 21:15:43, ngeoffray wrote: > weird identation. I believe the style is more: > > a && > foo && > ... > > Or: > > a > && foo > && ... > > I prefer the latter. Done. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... File lib/compiler/implementation/ssa/codegen.dart (right): https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/codegen.dart:1645: // separate step in the ssa optimizer. On 2012/09/25 21:15:43, ngeoffray wrote: > Please name the step. Done. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/codegen.dart:1649: backend.registerFieldSetter(work.element, node.element, types[node.value]); On 2012/09/25 21:15:43, ngeoffray wrote: > Line too long Done. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... File lib/compiler/implementation/ssa/optimize.dart (right): https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:29: void optimize(WorkItem work, HGraph graph, bool speculative) { On 2012/09/25 21:15:43, ngeoffray wrote: > I see no change of the callers of this method. They are in backend.dart line 680 and line 686. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1270: newFieldSetters[element] = type; On 2012/09/25 21:15:43, ngeoffray wrote: > Why are you setting it here? if currentFieldSetters doesn't have it, it means it > hasn't initialized it, right? So you should not add it to the newFieldSetters. > > Hmmm, if I'm right, maybe you got lucky with the tests. Adding a new one might > be helpful. Good catch. Test added. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1280: (HInstruction instruction) => instruction.accept(this)); On 2012/09/25 21:15:43, ngeoffray wrote: > You need to visit the phis, since they may use 'this'. Might need another test > too :) Added phi visiting and added 2 tests. https://codereview.chromium.org/10964016/diff/25001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1337: // For other fields having setters in the generative constructor body set On 2012/09/25 21:15:43, ngeoffray wrote: > body set -> body, set Done. https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... File tests/compiler/dart2js/field_type_inferer_test.dart (right): https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:203: A() { fs = 1; } On 2012/09/25 21:15:43, ngeoffray wrote: > Is that turned into a dynamic setter? If not, I can't find the code in your > change that prevents fs to be typed 'int'. The reason is that fs is set in A, but is defined in S. We only infer the type from the constructor if the field is set by the class defining it (see visitFieldSet). We might be able to lift that restriction but I would like to keep it as this right now. https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:279: runTest(TEST_8, {'f': HType.UNKNOWN}); On 2012/09/25 21:15:43, ngeoffray wrote: > unindent TEST_8 Done. https://codereview.chromium.org/10964016/diff/25001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:284: runTest(TEST_13, {'f': HType.UNKNOWN}); On 2012/09/25 21:15:43, ngeoffray wrote: > Add TODO(sgjesse): we should actually infer int. Done.
LGTM! Sorry about the comma comments :) https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... File lib/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:230: * For each class [constructors] holds the set of constructors. If there is each class, https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:232: * infer the field types from construction as the information collected does from construction, https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:236: final Map<Element, Set<Element>> constructors; Type the keys as ClassElement? https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:241: * types assigned in the constructor(s) [fieldConstructorTypeMap] and one , and https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:242: * for typesassigned in the rest of the code [fieldTypeMap]. typesassigned -> types assigned https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:242: * for typesassigned in the rest of the code [fieldTypeMap]. rest of the code where the field can be resolved. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:245: * list(s) then the type from the constructor(s) will owerride the one from , then https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:248: * As the order in which generative constructors, generative constructor As -> Because ? https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:249: * bodies and normal methods/functions is undefined, and as they can all be bodies, https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:249: * bodies and normal methods/functions is undefined, and as they can all be as -> because ? https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:250: * recompiled it is not possible to combile this information into one map at recompiled, https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:250: * recompiled it is not possible to combile this information into one map at combile -> combine https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:325: // When the second constructor is seen recompile all functions relying on seen, https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:394: return HType.UNKNOWN; Why don't you also check fieldTypeMap here? Please add a comment. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:396: HType result = constructorType != null ? constructorType : initializerType; Also add a comment here on why you're using constructorType first (the comment is already in optimize.dart, but it should also be there). https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... File lib/compiler/implementation/ssa/optimize.dart (right): https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1231: HInstruction thisInstruction; You don't need this field. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1292: thisExposed = thisExposed || thisUsers.contains(phi); We talked about adding the users of the phi to thisUsers if the phi is a user of 'this'. Is a test failing if you do such optimization? https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1314: includeSuperMembers: true, Please add a comment on why you (can?) include backend and super members. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1325: allSetters.add(field); As discussed, please add a comment here. https://codereview.chromium.org/10964016/diff/32001/tests/compiler/dart2js/fi... File tests/compiler/dart2js/field_type_inferer_test.dart (right): https://codereview.chromium.org/10964016/diff/32001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:292: A(x) { Add a comment that 'this' is exposed in the constructor and we don't infer anything because of it. https://codereview.chromium.org/10964016/diff/32001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:345: // TODO(sgjesse): We should actually infer int. infer int for TEST_13 and TEST_14
Addressed all comments. Also updated the status file related to issue 5517. Thanks for you time with this change - hopefully it will work! Committing. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... File lib/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:230: * For each class [constructors] holds the set of constructors. If there is On 2012/09/27 12:35:46, ngeoffray wrote: > each class, Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:232: * infer the field types from construction as the information collected does On 2012/09/27 12:35:46, ngeoffray wrote: > from construction, Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:236: final Map<Element, Set<Element>> constructors; On 2012/09/27 12:35:46, ngeoffray wrote: > Type the keys as ClassElement? Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:241: * types assigned in the constructor(s) [fieldConstructorTypeMap] and one On 2012/09/27 12:35:46, ngeoffray wrote: > , and Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:242: * for typesassigned in the rest of the code [fieldTypeMap]. On 2012/09/27 12:35:46, ngeoffray wrote: > typesassigned -> types assigned Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:242: * for typesassigned in the rest of the code [fieldTypeMap]. On 2012/09/27 12:35:46, ngeoffray wrote: > typesassigned -> types assigned Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:245: * list(s) then the type from the constructor(s) will owerride the one from On 2012/09/27 12:35:46, ngeoffray wrote: > , then Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:248: * As the order in which generative constructors, generative constructor On 2012/09/27 12:35:46, ngeoffray wrote: > As -> Because ? Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:249: * bodies and normal methods/functions is undefined, and as they can all be On 2012/09/27 12:35:46, ngeoffray wrote: > bodies, Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:249: * bodies and normal methods/functions is undefined, and as they can all be On 2012/09/27 12:35:46, ngeoffray wrote: > bodies, Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:250: * recompiled it is not possible to combile this information into one map at On 2012/09/27 12:35:46, ngeoffray wrote: > recompiled, Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:250: * recompiled it is not possible to combile this information into one map at On 2012/09/27 12:35:46, ngeoffray wrote: > recompiled, Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:325: // When the second constructor is seen recompile all functions relying on On 2012/09/27 12:35:46, ngeoffray wrote: > seen, Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:394: return HType.UNKNOWN; On 2012/09/27 12:35:46, ngeoffray wrote: > Why don't you also check fieldTypeMap here? Please add a comment. Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/js_backend/backend.dart:396: HType result = constructorType != null ? constructorType : initializerType; On 2012/09/27 12:35:46, ngeoffray wrote: > Also add a comment here on why you're using constructorType first (the comment > is already in optimize.dart, but it should also be there). Done. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... File lib/compiler/implementation/ssa/optimize.dart (right): https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1231: HInstruction thisInstruction; On 2012/09/27 12:35:46, ngeoffray wrote: > You don't need this field. Now I do - see below. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1292: thisExposed = thisExposed || thisUsers.contains(phi); On 2012/09/27 12:35:46, ngeoffray wrote: > We talked about adding the users of the phi to thisUsers if the phi is a user of > 'this'. Is a test failing if you do such optimization? That is actually why I still had the thisInstruction member above. Changed it to: if (phi.inputs.indexOf(thisInstruction) != -1) { thisUsers.addAll(phi.usedBy); } (can't remember why I changed it to the pessimistic version). Modified the expectations for TEST_18 and added TEST_19 where the phi is used in an invoke to really expose this. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1314: includeSuperMembers: true, On 2012/09/27 12:35:46, ngeoffray wrote: > Please add a comment on why you (can?) include backend and super members. Removed backend members and added comment. https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1325: allSetters.add(field); On 2012/09/27 12:35:46, ngeoffray wrote: > As discussed, please add a comment here. Done. https://codereview.chromium.org/10964016/diff/32001/tests/compiler/dart2js/fi... File tests/compiler/dart2js/field_type_inferer_test.dart (right): https://codereview.chromium.org/10964016/diff/32001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:292: A(x) { On 2012/09/27 12:35:46, ngeoffray wrote: > Add a comment that 'this' is exposed in the constructor and we don't infer > anything because of it. After changing the handling of Phis here this is no longer exposed and we can infer the types. Added TEST_19 where this is exposed (not just used in a field set). https://codereview.chromium.org/10964016/diff/32001/tests/compiler/dart2js/fi... tests/compiler/dart2js/field_type_inferer_test.dart:345: // TODO(sgjesse): We should actually infer int. On 2012/09/27 12:35:46, ngeoffray wrote: > infer int for TEST_13 and TEST_14 Done.
https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... File lib/compiler/implementation/ssa/optimize.dart (right): https://codereview.chromium.org/10964016/diff/32001/lib/compiler/implementati... lib/compiler/implementation/ssa/optimize.dart:1292: thisExposed = thisExposed || thisUsers.contains(phi); On 2012/09/27 13:22:49, Søren Gjesse wrote: > On 2012/09/27 12:35:46, ngeoffray wrote: > > We talked about adding the users of the phi to thisUsers if the phi is a user > of > > 'this'. Is a test failing if you do such optimization? > > That is actually why I still had the thisInstruction member above. Changed it > to: > > if (phi.inputs.indexOf(thisInstruction) != -1) { How about if (thisUsers.contains(phi)) ? > thisUsers.addAll(phi.usedBy); > } > > (can't remember why I changed it to the pessimistic version). > > Modified the expectations for TEST_18 and added TEST_19 where the phi is used in > an invoke to really expose this. |
