Recognize logical operations before code generation.

Recognize logical operations before code generation.
Committed: https://code.google.com/p/dart/source/detail?r=8247

[Lasse Reichstein Nielsen, 2012-06-04 11:09:22]

LGTM

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1626: // logical operation.
Is this method only called for phis that are generated at use site? If so, say
so somewhere, otherwise explain this comment.
It seems not all logical-operator phis are generate-at-use-site.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1630: use(node.inputs[1],
JSPrecedence.EXPRESSION_PRECEDENCE);
You should pass expectedPrecedence as second argument here and below.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen_helpers.dart (right):

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:32: && input is !HPhi) {
I've started preferring is!.
Or, alternatively, to please everybody: "is ! Phi" :P

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:151: if (instruction.block
!== block) return block.last !== block.first;
So you don't accept an expression spread over more than one block (e.g., another
&& or ||?)

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:160: // statement for it.
Not necessarily - we might use the comma-separator to generate an expression
anyway (but I don't think the previous code handled that either).

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:190: if
(end.predecessors[1] !== elseBlock) return;
This also doesn't accept a multi-block expression.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:196: if (elseBlock.first
!== elseBlock.last) return;
Also check that elseBlock ends in a goto.
Perhaps just 
  if (elseBlock.first is! HGoto (and not contiune/break)) return;

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/nodes.dart:930: bool isConstantTrue() => false;
This is getting ridiculous. Will isConstant0() be next?
I can see the reason to have a shorthand for
 x.isConstant() && x.asConstant().isTrue()
but maybe make it a separate function:
 bool isConstantTrue(HInstruction x) => ...
instead of putting them all on all HInstructions.
(On the other hand, it already includes so many other specialized functions, so
a few more is probably ok).

[kasperl, 2012-06-04 11:12:55]

LGTM.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:542: if
(logicalOperations.contains(info.condition.end.last)) return false;
Add a comment that explains what this does.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1270: return;
Maybe get rid of the two returns and add the last code under an else? If you
want to keep the returns, I would get rid of the else if (and just use if).

[ngeoffray, 2012-06-04 11:37:54]

Thank you Lasse and Kasper

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:542: if
(logicalOperations.contains(info.condition.end.last)) return false;
On 2012/06/04 11:12:55, kasperl wrote:
> Add a comment that explains what this does.

Done.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1270: return;
On 2012/06/04 11:12:55, kasperl wrote:
> Maybe get rid of the two returns and add the last code under an else? If you
> want to keep the returns, I would get rid of the else if (and just use if).

Removed the else if.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1626: // logical operation.
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> Is this method only called for phis that are generated at use site? If so, say
> so somewhere, otherwise explain this comment.
> It seems not all logical-operator phis are generate-at-use-site.

Done.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1630: use(node.inputs[1],
JSPrecedence.EXPRESSION_PRECEDENCE);
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> You should pass expectedPrecedence as second argument here and below.

Done.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen_helpers.dart (right):

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:32: && input is !HPhi) {
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> I've started preferring is!.
> Or, alternatively, to please everybody: "is ! HPhi"

Grmbl. What's wrong with everybody?

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:151: if (instruction.block
!== block) return block.last !== block.first;
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> So you don't accept an expression spread over more than one block (e.g.,
another
> && or ||?)

The graph is built in a way that multiple && or || follow the same pattern
described line 175. So what I would not recognize is probably:

(a ? b : c) && d

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:160: // statement for it.
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> Not necessarily - we might use the comma-separator to generate an expression
> anyway (but I don't think the previous code handled that either).

Added comment.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:190: if
(end.predecessors[1] !== elseBlock) return;
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> This also doesn't accept a multi-block expression.

Yes.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:196: if (elseBlock.first
!== elseBlock.last) return;
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> Also check that elseBlock ends in a goto.
> Perhaps just 
>   if (elseBlock.first is! HGoto (and not contiune/break)) return;

I have checked that the else block is the predecessor of the join block. I think
that covers it. As discussed, I added a comment.

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10495013/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/nodes.dart:930: bool isConstantTrue() => false;
On 2012/06/04 11:09:22, Lasse Reichstein Nielsen wrote:
> This is getting ridiculous. Will isConstant0() be next?
> I can see the reason to have a shorthand for
>  x.isConstant() && x.asConstant().isTrue()
> but maybe make it a separate function:
>  bool isConstantTrue(HInstruction x) => ...
> instead of putting them all on all HInstructions.
> (On the other hand, it already includes so many other specialized functions,
so
> a few more is probably ok).

I'm duplicating what we have in the Constant class. We'll stop from there :)