Generate prettier loops.

Generate prettier loops.

Add structure information to HLoopInformation, and use it to generate prettier
loops when the condition can be generated as an expression.
If it can't, it still falls back to the while(true){if(!cond) break;} pattern.
Only handle for/while/for-in, not do-while yet.

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

[floitsch, 2012-03-28 04:03:48]

LGTM.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (left):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:387: // TODO(floitsch): HACK HACK HACK.
WOOT!!! ;)

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:91: static final int STATE_FIRST_DECLARATION = 2;
FIRST_DECLARATION and STATE_DECLARATION are unused.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:227: print("[isExpression FAIL: $current]");
debug code?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:237: // Check the second half, which will continue
into the join.
Isn't the second half successors[1]?
Can't we just jump to the join-node?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:241: if (basicBlock !== limits.end)
print("[isExpression IF_FAIL: $current]");
debug code?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:245: print("[isExpression CC-FAIL: $current");
debug code?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:339: return (generationState & 0x01) == 0;
magic constant.
I don't see generationState as a bitfield. Is this just to avoid to test against
two constants?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:347: if (generationState <= STATE_FIRST_DECLARATION) {
if (generationState == STATE_FIRST_EXPRESSION) generationState = ...
else if (gen...

Your comment explains this, but I still need to verify it, and it is more
brittle (if we ever change the state).

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:495: print("LOOP:
[${["WHILE","FOR","DO-WHILE","FOR-IN"][info.type]}]");
debug code?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:551: continueActions[label] = continueAsBreak;
continueAction.
Add test.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:557: indent++;
move indent++ closer to "{" ?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:559: visitSubGraph(info.body);
I would keep the 'if' open and duplicate the 'visitSubGraph' line:
if (info.target === null || !info.target.isContinueTarget) {
  visitSubGraph(info.body);
} else {
  ...
  visitSubGraph(info.body);
  ...
}

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:578: return false;
I don't like these "return false/true" where it is difficult to see what
condition succeeded/failed to get to this line.
Maybe add a "bool succeeded = false" in the beginning and update it, when we
were able to handle it.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:606: if (handleLoop(node)) {
not a strong preference but I would prefer:
bool succeededNiceLoop = handleLoop(node);
currentBlockinformation = oldBlockInformation;
if (succeededNiceLoop) {
  visitBasicBlock(node.loopInformation.joinBlock);
  return;
}

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:620: bool firstExpression = true;
unused variable.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:899: if (isGeneratingExpression()) {
can you give an example when this is happening?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1128: if (isGeneratingExpression()) {
when can we enter here, but not generate an expression?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:848: // Does this node ootentially affect control flow.
potentially

[ngeoffray, 2012-03-28 15:28:04]

LGTM

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:225: // E.g, BailoutTarget.
BailoutTarget does not exist anymore.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1047: bool delayDeclaration = false;
Please add a comment on why FieldSet needs to participate so actively in the
delayed variable declaration.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1127: if (subGraph !== null && node.block ==
subGraph.end) {
How can that happen? Please add a comment.

[Lasse Reichstein Nielsen, 2012-03-29 13:09:38]

PTanotherL

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (left):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:387: // TODO(floitsch): HACK HACK HACK.
Don't be too happy, the code is still there as a fallback in case the handleLoop
can't do it's job.

Eventually, I want to move the fall-back code to handleLoop too, and do it using
subgraphs, but it's not there yet.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:91: static final int STATE_FIRST_DECLARATION = 2;
Yes. They are preparing for including the initializer part of a for loop.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:227: print("[isExpression FAIL: $current]");
Ah, yes. Will remove them all.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:237: // Check the second half, which will continue
into the join.
No, successors[1] is the join node, the first half is inputs[0].

Checking the second half is really a sanity check. It should be all expressions
or we wouldn't have made the logical operation inlinable anyway. I prefer not to
rely too much on the preconditions for the current state (i.e., the
HConditionalBranch is generateAtUseSite - I allow myself to know what that
means, but not to make assumptions about how it was decided).

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:339: return (generationState & 0x01) == 0;
Yep. Fixed.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:347: if (generationState <= STATE_FIRST_DECLARATION) {
On 2012/03/28 04:03:48, floitsch wrote:
> if (generationState == STATE_FIRST_EXPRESSION) generationState = ...
> else if (gen...
> 
> Your comment explains this, but I still need to verify it, and it is more
> brittle (if we ever change the state).

Done.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:557: indent++;
Refactored away.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:559: visitSubGraph(info.body);
On 2012/03/28 04:03:48, floitsch wrote:
> I would keep the 'if' open and duplicate the 'visitSubGraph' line:
> if (info.target === null || !info.target.isContinueTarget) {
>   visitSubGraph(info.body);
> } else {
>   ...
>   visitSubGraph(info.body);
>   ...
> }

Done.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:578: return false;
On 2012/03/28 04:03:48, floitsch wrote:
> I don't like these "return false/true" where it is difficult to see what
> condition succeeded/failed to get to this line.
> Maybe add a "bool succeeded = false" in the beginning and update it, when we
> were able to handle it.

Done.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:606: if (handleLoop(node)) {
On 2012/03/28 04:03:48, floitsch wrote:
> not a strong preference but I would prefer:
> bool succeededNiceLoop = handleLoop(node);
> currentBlockinformation = oldBlockInformation;
> if (succeededNiceLoop) {
>   visitBasicBlock(node.loopInformation.joinBlock);
>   return;
> }

Done.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:620: bool firstExpression = true;
On 2012/03/28 04:03:48, floitsch wrote:
> unused variable.

Done.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:899: if (isGeneratingExpression()) {
When we are expression-generating the condition of a loop (via
visitConditionSubGraph(SubGraph)).

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1047: bool delayDeclaration = false;
Even better, instead of deferring it to a later cleanup, I have abstracted the
var-introduction into a separate method. Made everything much more readable.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1127: if (subGraph !== null && node.block ==
subGraph.end) {
On 2012/03/28 15:28:04, ngeoffray wrote:
> How can that happen? Please add a comment.

Done.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1128: if (isGeneratingExpression()) {
We can't yet.
Eventually I'll want to handle loops with non-expression-able conditions using
subgraph traversal too, and then we'll get here when traversing the condition
subgraph as statements.

Should I remove the special case here and always use the condition expression at
the visitExpressionSubGraph call point?

[floitsch, 2012-03-29 21:44:10]

SLGTM.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:237: // Check the second half, which will continue
into the join.
On 2012/03/29 13:09:38, Lasse Reichstein Nielsen wrote:
> No, successors[1] is the join node, the first half is inputs[0].
Right... missed that. Please add as comment.
> 
> Checking the second half is really a sanity check. It should be all
expressions
> or we wouldn't have made the logical operation inlinable anyway. I prefer not
to
> rely too much on the preconditions for the current state (i.e., the
> HConditionalBranch is generateAtUseSite - I allow myself to know what that
> means, but not to make assumptions about how it was decided).

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:899: if (isGeneratingExpression()) {
On 2012/03/29 13:09:38, Lasse Reichstein Nielsen wrote:
> When we are expression-generating the condition of a loop (via
> visitConditionSubGraph(SubGraph)).

But then it should be a loop-condition and end with a LoopBranch. When is this
not the case?

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1128: if (isGeneratingExpression()) {
On 2012/03/29 13:09:38, Lasse Reichstein Nielsen wrote:
> We can't yet.
> Eventually I'll want to handle loops with non-expression-able conditions using
> subgraph traversal too, and then we'll get here when traversing the condition
> subgraph as statements.
> 
> Should I remove the special case here and always use the condition expression
at
> the visitExpressionSubGraph call point?

If we can't reach this unless it's an expression, make it an internal error if
it is not an expression. You won't lose any code, and it will be easy to change
once it can happen.

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/codegen...
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/codegen...
frog/leg/ssa/codegen.dart:854: buffer.add("/*b*/");
debug?

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/codegen...
frog/leg/ssa/codegen.dart:856: buffer.add("/*a*/");
ditto.

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/nodes.d...
frog/leg/ssa/nodes.dart:2002: if (element === null) throw "WHAT?!?";
debug?

[Lasse Reichstein Nielsen, 2012-03-30 09:37:17]

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:237: // Check the second half, which will continue
into the join.
On 2012/03/29 21:44:10, floitsch wrote:
> On 2012/03/29 13:09:38, Lasse Reichstein Nielsen wrote:
> > No, successors[1] is the join node, the first half is inputs[0].
> Right... missed that. Please add as comment.
> > 
> > Checking the second half is really a sanity check. It should be all
> expressions
> > or we wouldn't have made the logical operation inlinable anyway. I prefer
not
> to
> > rely too much on the preconditions for the current state (i.e., the
> > HConditionalBranch is generateAtUseSite - I allow myself to know what that
> > means, but not to make assumptions about how it was decided).
> 

Done.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:899: if (isGeneratingExpression()) {
True. A HIf is only part of an expression if it's a logical operator, and that's
handled elsewhere.
I'll remove this.

https://chromiumcodereview.appspot.com/9863037/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1128: if (isGeneratingExpression()) {
On 2012/03/29 21:44:10, floitsch wrote:
> On 2012/03/29 13:09:38, Lasse Reichstein Nielsen wrote:
> > We can't yet.
> > Eventually I'll want to handle loops with non-expression-able conditions
using
> > subgraph traversal too, and then we'll get here when traversing the
condition
> > subgraph as statements.
> > 
> > Should I remove the special case here and always use the condition
expression
> at
> > the visitExpressionSubGraph call point?
> 
> If we can't reach this unless it's an expression, make it an internal error if
> it is not an expression. You won't lose any code, and it will be easy to
change
> once it can happen.

Done.

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/codegen...
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/codegen...
frog/leg/ssa/codegen.dart:854: buffer.add("/*b*/");
On 2012/03/29 21:44:11, floitsch wrote:
> debug?

Done.

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/codegen...
frog/leg/ssa/codegen.dart:856: buffer.add("/*a*/");
On 2012/03/29 21:44:11, floitsch wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9863037/diff/6001/frog/leg/ssa/nodes.d...
frog/leg/ssa/nodes.dart:2002: if (element === null) throw "WHAT?!?";
On 2012/03/29 21:44:11, floitsch wrote:
> debug?

Done.