Avoid generating empty else.

Avoid generating empty else.
Committed: https://code.google.com/p/dart/source/detail?r=8149

[kasperl, 2012-05-31 11:26:34]

LGTM.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:558:
generateStatements(info.elseGraph);
info.elseGraph -> elseGraph

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1257: bool hasCodeUntil(HBasicBlock
block, HBasicBlock successor) {
Not sure I like the name. Maybe negate it and make it more specific like
isEmptyElseBlock(block, join)?

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1258: if (block.last is !HGoto)
return true;
If the last instruction is a goto can the block have more than one successor?

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1261: HInstruction instruction =
block.first;
Add a comment saying that we generate at use site instructions are okay because
they do not generate code in this else block.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1269: for (Copy copy in
handler.copies) {
Add a comment that briefly explains what this loop checks for.

[ngeoffray, 2012-05-31 11:51:28]

Thanks Kasper

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:558:
generateStatements(info.elseGraph);
On 2012/05/31 11:26:34, kasperl wrote:
> info.elseGraph -> elseGraph

Done.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1257: bool hasCodeUntil(HBasicBlock
block, HBasicBlock successor) {
On 2012/05/31 11:26:34, kasperl wrote:
> Not sure I like the name. Maybe negate it and make it more specific like
> isEmptyElseBlock(block, join)?

Done.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1258: if (block.last is !HGoto)
return true;
On 2012/05/31 11:26:34, kasperl wrote:
> If the last instruction is a goto can the block have more than one successor?

Good point! Check removed.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1261: HInstruction instruction =
block.first;
On 2012/05/31 11:26:34, kasperl wrote:
> Add a comment saying that we generate at use site instructions are okay
because
> they do not generate code in this else block.

Done.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1269: for (Copy copy in
handler.copies) {
On 2012/05/31 11:26:34, kasperl wrote:
> Add a comment that briefly explains what this loop checks for.

Done.

[Lasse Reichstein Nielsen, 2012-05-31 11:57:27]

LGTM with comments.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:553: HIf ifInstruction =
info.thenGraph.start.predecessors[0].last;
Or:
 HIf ifInstruction = info.condition.conditionExpression;

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:555: &&
hasCodeUntil(elseGraph.start, ifInstruction.joinBlock)) {
I really, really don't like using the ifInstruction's joinBlock in the
block-structure based handling. 

How about working on the closed interval that is the elseGraph, i.e.
  hasCodeUntil(elseGraph.start, elseGraph.end) 
here and
  hasCodeUntil(node.elseBlock, node.joinBlock.predecessor[1])
below?

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1258: if (block.last is !HGoto)
return true;
Sadly, HBreak and HContinue are subclasses of HGoto.
We should probably change that, it keeps tripping me up too.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1262: while (instruction !=
block.last) {
for-loop?

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1272: if (sourceName !=
destinationName) return true;
Would it make sense to remove these "identity-copies", or is there a reason to
keep them?

[ngeoffray, 2012-05-31 12:27:38]

Thanks Lasse.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:553: HIf ifInstruction =
info.thenGraph.start.predecessors[0].last;
On 2012/05/31 11:57:28, Lasse Reichstein Nielsen wrote:
> Or:
>  HIf ifInstruction = info.condition.conditionExpression;

Done.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:555: &&
hasCodeUntil(elseGraph.start, ifInstruction.joinBlock)) {
On 2012/05/31 11:57:28, Lasse Reichstein Nielsen wrote:
> I really, really don't like using the ifInstruction's joinBlock in the
> block-structure based handling. 
> 
> How about working on the closed interval that is the elseGraph, i.e.
>   hasCodeUntil(elseGraph.start, elseGraph.end) 
> here and
>   hasCodeUntil(node.elseBlock, node.joinBlock.predecessor[1])
> below? 

Done.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1258: if (block.last is !HGoto)
return true;
On 2012/05/31 11:57:28, Lasse Reichstein Nielsen wrote:
> Sadly, HBreak and HContinue are subclasses of HGoto.
> We should probably change that, it keeps tripping me up too.

+1

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1262: while (instruction !=
block.last) {
On 2012/05/31 11:57:28, Lasse Reichstein Nielsen wrote:
> for-loop?

Done.

https://chromiumcodereview.appspot.com/10446098/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1272: if (sourceName !=
destinationName) return true;
On 2012/05/31 11:57:28, Lasse Reichstein Nielsen wrote:
> Would it make sense to remove these "identity-copies", or is there a reason to
> keep them?

At the time where we create the copies of HInstruction, we actually don't know
if they are identity copies. I could remove them once we have done the
allocation of variable names, but it felt just easier to just let the algorithm
that sequentializes the copies do it.