Set up a stub pipline for using the new cps-based ir to generate js.

Set up a stub pipline for using the new cps-based ir to generate js.

The code is only activated when enabling
environment variable USE_CPS_IR.

R=floitsch@google.com

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

[sigurdm, 2014-11-11 15:25:14]

Patchset #2 (id:20001) has been deleted

[sigurdm, 2014-11-11 15:27:48]

sigurdm@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-11-12 11:03:20]

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/com...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:719: static const bool USE_CPS_IR = const
bool.fromEnvironment("USE_CPS_IR");
Just move it to js_backend/backend.dart unless you think other parts of dart2js
need to have access to it. (But they shouldn't).

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:56: if
(!irEnabled(useNewBackend: useNewBackend)) return;
Why does the builder need to know about backends?
It looks to me like this should have been checked before.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:453: ? new CodeGenTask(compiler)
I prefer "Codegen", but I can see why the "G" should be capitalized.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:1210: jsAst.Expression code;
Alternatively we could:
- have one "CodegenTask" in this class.
- Make that one either a SsaCodegenTask or a CpsCodegenTask.
- let the SsaCodegenTask do the optimizer.optimize + get generator.generateCode.
- let the CpsCodegenTask fall back to the SsaCodegen if it can't handle the
code. That is, it would always return valid code and the backend wouldn't need
to know that the CpsCodegen isn't complete yet. (The code here, would then live
in the CpsCodegenTask).

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:22: Map<VariableElement,
String> constantNames = <VariableElement, String>{};
Maplet?

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:35: void
buildFunction(tree_ir.FunctionDefinition function) {
This is the main entry point. Right?
Add dartdoc here.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/task.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:10: import
'../../cps_ir/cps_ir_nodes.dart' as cps_ir;
I would go with "as cps".

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:30: Tracer tracer; //
TODO(sigurdm): Assign this.
nit: two spaces before "//".

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:42: js.Fun
build(CodegenWorkItem work) {
This is the public entry point.
Add dartdoc.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:47: cps_ir.FunctionDefinition
cps_definition;
cpsDefinition

Probably a copy/paste. Please fix the source too.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:47: cps_ir.FunctionDefinition
cps_definition;
cpsDefinition =
    irBuilderTask.buildNode(element);

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:53:
tracer.traceCompilation(element.name, null);
The cps_ir_builder_visitor.dart already does a tracer.traceCompilation.
Are we duplicating the tracing?

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:96: codeGen.parameters,
I don't like to return values as fields.
Let's revisit that in a later CL.

[sigurdm, 2014-11-13 08:29:42]

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/com...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:719: static const bool USE_CPS_IR = const
bool.fromEnvironment("USE_CPS_IR");
On 2014/11/12 11:03:19, floitsch wrote:
> Just move it to js_backend/backend.dart unless you think other parts of
dart2js
> need to have access to it. (But they shouldn't).

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:56: if
(!irEnabled(useNewBackend: useNewBackend)) return;
On 2014/11/12 11:03:19, floitsch wrote:
> Why does the builder need to know about backends?
> It looks to me like this should have been checked before.

I agree - I removed it - it was only used from tests.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:453: ? new CodeGenTask(compiler)
On 2014/11/12 11:03:20, floitsch wrote:
> I prefer "Codegen", but I can see why the "G" should be capitalized.

It is renamed to CodeGenerator in the next CL.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:1210: jsAst.Expression code;
On 2014/11/12 11:03:20, floitsch wrote:
> Alternatively we could:
> - have one "CodegenTask" in this class.
> - Make that one either a SsaCodegenTask or a CpsCodegenTask.
> - let the SsaCodegenTask do the optimizer.optimize + get
generator.generateCode.
> - let the CpsCodegenTask fall back to the SsaCodegen if it can't handle the
> code. That is, it would always return valid code and the backend wouldn't need
> to know that the CpsCodegen isn't complete yet. (The code here, would then
live
> in the CpsCodegenTask).

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:22: Map<VariableElement,
String> constantNames = <VariableElement, String>{};
On 2014/11/12 11:03:20, floitsch wrote:
> Maplet?

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:35: void
buildFunction(tree_ir.FunctionDefinition function) {
On 2014/11/12 11:03:20, floitsch wrote:
> This is the main entry point. Right?
> Add dartdoc here.

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/task.dart (right):

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:10: import
'../../cps_ir/cps_ir_nodes.dart' as cps_ir;
On 2014/11/12 11:03:20, floitsch wrote:
> I would go with "as cps".

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:30: Tracer tracer; //
TODO(sigurdm): Assign this.
On 2014/11/12 11:03:20, floitsch wrote:
> nit: two spaces before "//".

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:42: js.Fun
build(CodegenWorkItem work) {
On 2014/11/12 11:03:20, floitsch wrote:
> This is the public entry point.
> Add dartdoc.

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:47: cps_ir.FunctionDefinition
cps_definition;
On 2014/11/12 11:03:20, floitsch wrote:
> cpsDefinition =
>     irBuilderTask.buildNode(element);

Done.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:47: cps_ir.FunctionDefinition
cps_definition;
On 2014/11/12 11:03:20, floitsch wrote:
> cpsDefinition
> 
> Probably a copy/paste. Please fix the source too.

Seems to be my mistake only

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:53:
tracer.traceCompilation(element.name, null);
On 2014/11/12 11:03:20, floitsch wrote:
> The cps_ir_builder_visitor.dart already does a tracer.traceCompilation.
> Are we duplicating the tracing?

It seems so - I disabled the tracing from cps_ir_builder_visitor.dart.

https://codereview.chromium.org/716823002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/task.dart:96: codeGen.parameters,
On 2014/11/12 11:03:20, floitsch wrote:
> I don't like to return values as fields.
> Let's revisit that in a later CL.

Acknowledged.

[sigurdm, 2014-11-13 08:41:16]

Patchset #3 (id:60001) has been deleted

[floitsch, 2014-11-13 14:30:55]

LGTM.

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1492: log('Building IR...');
move log into if.

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:52: if (!irEnabled())
return;
We should not call `buildNodes` if we aren't interested in its results.

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:67:
!compiler.enableTypeAssertions &&
These checks should probably go to the compiler too.

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/task.dart (right):

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/task.dart:43: FunctionCompiler
fallbackPipeline;
fallbackCompiler?

Maybe add a (non-dartdoc) comment that we need to update the `compile` method
dartdoc when removing this field.

[sigurdm, 2014-11-13 14:43:42]

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1492: log('Building IR...');
On 2014/11/13 14:30:55, floitsch wrote:
> move log into if.

Done.

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:52: if (!irEnabled())
return;
On 2014/11/13 14:30:55, floitsch wrote:
> We should not call `buildNodes` if we aren't interested in its results.

Done.

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:67:
!compiler.enableTypeAssertions &&
On 2014/11/13 14:30:55, floitsch wrote:
> These checks should probably go to the compiler too.

TODO added

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/task.dart (right):

https://codereview.chromium.org/716823002/diff/100001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/task.dart:43: FunctionCompiler
fallbackPipeline;
On 2014/11/13 14:30:55, floitsch wrote:
> fallbackCompiler?
> 
> Maybe add a (non-dartdoc) comment that we need to update the `compile` method
> dartdoc when removing this field.

done

[sigurdm, 2014-11-13 14:57:53]

Committed patchset #6 (id:140001) manually as 41716 (presubmit successful).