Integrity checker for CPS and Tree IR.

Integrity checker for CPS and Tree IR.

BUG=
R=kmillikin@google.com

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

[asgerf, 2015-03-04 11:36:16]

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

[asgerf, 2015-03-04 11:36:17]

CLs that fix integrity violations:
- https://codereview.chromium.org/979753002 (for loops)
- https://codereview.chromium.org/979743002 (CPS visitors)
- https://codereview.chromium.org/970853006 (reference counting)
- https://codereview.chromium.org/977553004 (constructors)

https://codereview.chromium.org/981523002/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/codegen/task.dart (left):

https://codereview.chromium.org/981523002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/task.dart:191: new
backend_ast_emitter.UnshadowParameters().unshadow(treeNode);
I removed the UnshadowParameters phase as a drive-by change. It is specific to
the Dart backend and this is the JS backend.

https://codereview.chromium.org/981523002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/981523002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:858: }
I plan to steamline the Visitor in another CL. It's getting messy.

[Kevin Millikin (Google), 2015-03-04 13:07:41]

LGTM with suggestions.

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/dar...
File pkg/compiler/lib/src/dart_backend/backend.dart (right):

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/dar...
pkg/compiler/lib/src/dart_backend/backend.dart:154: void applyCpsPass(String
name, cps_opt.Pass pass) {
This might be part of the rewrite method in the PassMixin and use a name getter
defined in the pass classes?  Somehow it just seems redundant to pass both a
name string and the pass itself.

And it's not clear where future 'wrapping' code should go.

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/tre...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/tre...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:854: if (node is
ConstructorDefinition) return visitConstructorDefinition(node);
I think it would be better to come up with a way to have accept methods for all
classes C in { ConstructorDefinition, FunctionDefinition, FieldDefinition,
FieldInitializer, SuperInitializer }; and to have visitC abstract methods in
class Visitor, and then in class Visitor, visitExecutableDefinition could be
d.accept(this).

This way makes me nervous, especially the order of comparisons
(constructorDefinition extends FunctionDefinition).

[asgerf, 2015-03-04 13:29:12]

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/dar...
File pkg/compiler/lib/src/dart_backend/backend.dart (right):

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/dar...
pkg/compiler/lib/src/dart_backend/backend.dart:154: void applyCpsPass(String
name, cps_opt.Pass pass) {
On 2015/03/04 13:07:41, kmillikin wrote:
> This might be part of the rewrite method in the PassMixin and use a name
getter
> defined in the pass classes?  Somehow it just seems redundant to pass both a
> name string and the pass itself.
> 
> And it's not clear where future 'wrapping' code should go.

I don't like calling traceGraph from inside the Pass since it depends on the
global monster state. I've added a name to passes though.

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/tre...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/981523002/diff/20001/pkg/compiler/lib/src/tre...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:854: if (node is
ConstructorDefinition) return visitConstructorDefinition(node);
On 2015/03/04 13:07:41, kmillikin wrote:
> I think it would be better to come up with a way to have accept methods for
all
> classes C in { ConstructorDefinition, FunctionDefinition, FieldDefinition,
> FieldInitializer, SuperInitializer }; and to have visitC abstract methods in
> class Visitor, and then in class Visitor, visitExecutableDefinition could be
> d.accept(this).
> 
> This way makes me nervous, especially the order of comparisons
> (constructorDefinition extends FunctionDefinition).

Yes, it's horrible. I put a comment in the CL about this, but it seems to be
invisible here because I uploaded a patch afterwards :(

In any case: I plan to fix the tree visitor in another CL. The Pass is sort of
acting as a visitor for top-level nodes, but the visitor should be the visitor,
not the pass.

I've added a TODO for now.

[Kevin Millikin (Google), 2015-03-04 13:34:38]

> Yes, it's horrible. I put a comment in the CL about this, but it seems to be
> invisible here because I uploaded a patch afterwards :(
> 
> In any case: I plan to fix the tree visitor in another CL. The Pass is sort of
> acting as a visitor for top-level nodes, but the visitor should be the
visitor,
> not the pass.
> 
> I've added a TODO for now.

Sounds good to me.

[karlklose, 2015-03-05 10:49:58]

On 2015/03/04 13:34:38, kmillikin wrote:
> > Yes, it's horrible. I put a comment in the CL about this, but it seems to be
> > invisible here because I uploaded a patch afterwards :(
> > 
> > In any case: I plan to fix the tree visitor in another CL. The Pass is sort
of
> > acting as a visitor for top-level nodes, but the visitor should be the
> visitor,
> > not the pass.
> > 
> > I've added a TODO for now.
> 
> Sounds good to me.

Didn't you mention a checked mode crash, for which you need to update the status
file?

[asgerf, 2015-03-05 13:25:21]

PTAL at delta from patch 5 to 4.

@karl: yes, status file has been updated.

[Kevin Millikin (Google), 2015-03-06 09:43:37]

Looks even better.

[asgerf, 2015-03-06 11:28:06]

Committed patchset #5 (id:80001) manually as 44285 (presubmit successful).