kernel -> ssa: implement if-statements

kernel -> ssa: implement if-statements

As part of this change I refactored SsaBranchBuilder to accept a
GraphBuilder argument so that it can work with the new kernel builder.

I also had to add a LocalsHandler accessor to the GraphBuilder.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/7502743614fcf22a943923f090f52736b45398ea

[Harry Terkelsen, 2016-09-01 23:19:22]

het@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-09-01 23:51:22]

sweet!

some comments below

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:2593: void
handleLogicalOrWithLeftNode(ast.Node left, void visitRight()) {
what if we keep the handleLogicalAndOrWithLeftNode function that takes `isAnd`
but move it here to builder?

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:2604: isAnd: true);
shouldn't this be "false"?

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:119: // TODO(het): add boolean
conversion type check
once you do, would this method move to GraphBuilder?

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:126: // allow [GraphBuilder] to
access `compiler`?
seems ok, is it only for boolType? We could also just pass that to the builder

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
File pkg/compiler/lib/src/ssa/ssa_branch_builder.dart (right):

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
pkg/compiler/lib/src/ssa/ssa_branch_builder.dart:1: import '../compiler.dart';
+ copyright

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
pkg/compiler/lib/src/ssa/ssa_branch_builder.dart:134: void
handleLogicalAndOr(void left(), void right(), {bool isAnd}) {
consider making this private and exposing 2 public functions:
handleLogicalAnd(l, r) => _handleLogicalBinary(l, r, isAnd: true)
handleLogicalOr(l, r) => _handleLogicalBinary(l, r, isAnd: false)

https://codereview.chromium.org/2301293002/diff/1/tests/compiler/dart2js/kern...
File tests/compiler/dart2js/kernel/control_flow_test.dart (right):

https://codereview.chromium.org/2301293002/diff/1/tests/compiler/dart2js/kern...
tests/compiler/dart2js/kernel/control_flow_test.dart:14: if (true) {
does this generate the condition (or are we eliminating them after
optimizations?)

Can we generate a couple tests with &&, ||, double && &&  and double || ||?

[Harry Terkelsen, 2016-09-02 17:52:28]

Thanks, Siggi!

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:2593: void
handleLogicalOrWithLeftNode(ast.Node left, void visitRight()) {
On 2016/09/01 23:51:21, Siggi Cherem (dart-lang) wrote:
> what if we keep the handleLogicalAndOrWithLeftNode function that takes `isAnd`
> but move it here to builder?

Done.

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:2604: isAnd: true);
On 2016/09/01 23:51:21, Siggi Cherem (dart-lang) wrote:
> shouldn't this be "false"?

oops! good catch

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:119: // TODO(het): add boolean
conversion type check
On 2016/09/01 23:51:21, Siggi Cherem (dart-lang) wrote:
> once you do, would this method move to GraphBuilder?

Yes, if we also pass in the boolType

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder_kernel.dart:126: // allow [GraphBuilder] to
access `compiler`?
On 2016/09/01 23:51:22, Siggi Cherem (dart-lang) wrote:
> seems ok, is it only for boolType? We could also just pass that to the builder

It's for boolType and for the 'compiler' argument to graph.addConstantNull

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
File pkg/compiler/lib/src/ssa/ssa_branch_builder.dart (right):

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
pkg/compiler/lib/src/ssa/ssa_branch_builder.dart:1: import '../compiler.dart';
On 2016/09/01 23:51:22, Siggi Cherem (dart-lang) wrote:
> + copyright

Done.

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
pkg/compiler/lib/src/ssa/ssa_branch_builder.dart:134: void
handleLogicalAndOr(void left(), void right(), {bool isAnd}) {
On 2016/09/01 23:51:22, Siggi Cherem (dart-lang) wrote:
> consider making this private and exposing 2 public functions:
> handleLogicalAnd(l, r) => _handleLogicalBinary(l, r, isAnd: true)
> handleLogicalOr(l, r) => _handleLogicalBinary(l, r, isAnd: false)

Seems good, but if I do this then I will have to change
handleLogicalAndOrWithLeftNode in builder to have something like:

if (isAnd) {
  handleLogicalAnd(...);
else {
  handleLogicalOr(...);
}

https://codereview.chromium.org/2301293002/diff/1/tests/compiler/dart2js/kern...
File tests/compiler/dart2js/kernel/control_flow_test.dart (right):

https://codereview.chromium.org/2301293002/diff/1/tests/compiler/dart2js/kern...
tests/compiler/dart2js/kernel/control_flow_test.dart:14: if (true) {
On 2016/09/01 23:51:22, Siggi Cherem (dart-lang) wrote:
> does this generate the condition (or are we eliminating them after
> optimizations?)
> 
> Can we generate a couple tests with &&, ||, double && &&  and double || ||?

we can't compile logical operators yet, but I will add them in the next cl

[Siggi Cherem (dart-lang), 2016-09-02 17:56:37]

lgtm, thanks harry!

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
File pkg/compiler/lib/src/ssa/ssa_branch_builder.dart (right):

https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
pkg/compiler/lib/src/ssa/ssa_branch_builder.dart:134: void
handleLogicalAndOr(void left(), void right(), {bool isAnd}) {
On 2016/09/02 17:52:27, Harry Terkelsen wrote:
> On 2016/09/01 23:51:22, Siggi Cherem (dart-lang) wrote:
> > consider making this private and exposing 2 public functions:
> > handleLogicalAnd(l, r) => _handleLogicalBinary(l, r, isAnd: true)
> > handleLogicalOr(l, r) => _handleLogicalBinary(l, r, isAnd: false)
> 
> Seems good, but if I do this then I will have to change
> handleLogicalAndOrWithLeftNode in builder to have something like:
> 
> if (isAnd) {
>   handleLogicalAnd(...);
> else {
>   handleLogicalOr(...);
> }

Good point - now that we refactored the other, it makes sense to keep this
generic one. What do you think between the two names "handleLogicalAndOr" or
"handleLogicalBinary" (and say this is && or || in the dartdoc?

[Harry Terkelsen, 2016-09-02 18:12:44]

On 2016/09/02 17:56:37, Siggi Cherem (dart-lang) wrote:
> lgtm, thanks harry!
> 
>
https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
> File pkg/compiler/lib/src/ssa/ssa_branch_builder.dart (right):
> 
>
https://codereview.chromium.org/2301293002/diff/1/pkg/compiler/lib/src/ssa/ss...
> pkg/compiler/lib/src/ssa/ssa_branch_builder.dart:134: void
> handleLogicalAndOr(void left(), void right(), {bool isAnd}) {
> On 2016/09/02 17:52:27, Harry Terkelsen wrote:
> > On 2016/09/01 23:51:22, Siggi Cherem (dart-lang) wrote:
> > > consider making this private and exposing 2 public functions:
> > > handleLogicalAnd(l, r) => _handleLogicalBinary(l, r, isAnd: true)
> > > handleLogicalOr(l, r) => _handleLogicalBinary(l, r, isAnd: false)
> > 
> > Seems good, but if I do this then I will have to change
> > handleLogicalAndOrWithLeftNode in builder to have something like:
> > 
> > if (isAnd) {
> >   handleLogicalAnd(...);
> > else {
> >   handleLogicalOr(...);
> > }
> 
> Good point - now that we refactored the other, it makes sense to keep this
> generic one. What do you think between the two names "handleLogicalAndOr" or
> "handleLogicalBinary" (and say this is && or || in the dartdoc?

I like handleLogicalBinary better. I renamed it in this CL. I also moved the
dartdoc

[Harry Terkelsen, 2016-09-02 18:31:01]

PTAL Siggi, I had to disable the binary_operators_test since we now are
compiling 'if' statements but not 'is' checks

[Siggi Cherem (dart-lang), 2016-09-02 18:32:15]

lgtm

[Harry Terkelsen, 2016-09-02 18:32:57]

Description was changed from

==========
kernel -> ssa: implement if-statements

As part of this change I refactored SsaBranchBuilder to accept a
GraphBuilder argument so that it can work with the new kernel builder.

I also had to add a LocalsHandler accessor to the GraphBuilder.
==========

to

==========
kernel -> ssa: implement if-statements

As part of this change I refactored SsaBranchBuilder to accept a
GraphBuilder argument so that it can work with the new kernel builder.

I also had to add a LocalsHandler accessor to the GraphBuilder.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7502743614fcf22a943923f090f52736b45398ea
==========

[Harry Terkelsen, 2016-09-02 18:32:58]

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