Avoid generating empty else.

lib/compiler/implementation/ssa/codegen.dart

Index: lib/compiler/implementation/ssa/codegen.dart
===================================================================
--- lib/compiler/implementation/ssa/codegen.dart	(revision 8145)
+++ lib/compiler/implementation/ssa/codegen.dart	(working copy)
@@ -549,7 +549,10 @@
       generateStatements(info.thenGraph);
       indent--;
       addIndented("}");
-      if (info.elseGraph !== null) {
+      HSubGraphBlockInformation elseGraph = info.elseGraph;
+      HIf ifInstruction = info.thenGraph.start.predecessors[0].last;

[Lasse Reichstein Nielsen, 2012/05/31 11:57:28]

Or:
 HIf ifInstruction = info.condition.conditionExpression;

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

Done.

+      if (elseGraph !== null
+          && hasCodeUntil(elseGraph.start, ifInstruction.joinBlock)) {

[Lasse Reichstein Nielsen, 2012/05/31 11:57:28]

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? 

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

Done.

         buffer.add(" else {\n");
         indent++;
         generateStatements(info.elseGraph);

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

info.elseGraph -> elseGraph

[ngeoffray, 2012/05/31 11:51:29]

Done.

@@ -1251,6 +1254,26 @@
     compiler.internalError('visitTry should not be called', instruction: node);
   }
 
+  bool hasCodeUntil(HBasicBlock block, HBasicBlock successor) {

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

Not sure I like the name. Maybe negate it and make it more specific like
isEmptyElseBlock(block, join)?

[ngeoffray, 2012/05/31 11:51:29]

Done.

+    if (block.last is !HGoto) return true;

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

If the last instruction is a goto can the block have more than one successor?

[ngeoffray, 2012/05/31 11:51:29]

Good point! Check removed.

[Lasse Reichstein Nielsen, 2012/05/31 11:57:28]

Sadly, HBreak and HContinue are subclasses of HGoto.
We should probably change that, it keeps tripping me up too.

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

+1

+    if (block.successors.length != 1) return true;
+    if (block.successors[0] !== successor) return true;
+    HInstruction instruction = block.first;

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

Add a comment saying that we generate at use site instructions are okay because
they do not generate code in this else block.

[ngeoffray, 2012/05/31 11:51:29]

Done.

+    while (instruction != block.last) {

[Lasse Reichstein Nielsen, 2012/05/31 11:57:28]

for-loop?

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

Done.

+      if (!isGenerateAtUseSite(instruction)) return true;
+      instruction = instruction.next;
+    }
+    CopyHandler handler = variableNames.getCopyHandler(block);
+    if (handler == null || handler.isEmpty()) return false;
+    if (!handler.assignments.isEmpty()) return true;
+    for (Copy copy in handler.copies) {

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

Add a comment that briefly explains what this loop checks for.

[ngeoffray, 2012/05/31 11:51:29]

Done.

+      String sourceName = variableNames.getName(copy.source);
+      String destinationName = variableNames.getName(copy.destination);
+      if (sourceName != destinationName) return true;

[Lasse Reichstein Nielsen, 2012/05/31 11:57:28]

Would it make sense to remove these "identity-copies", or is there a reason to
keep them?

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

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.

+    }
+    return false;
+  }
+
   visitIf(HIf node) {
     if (subGraph !== null && node.block === subGraph.end) {
       if (isGeneratingExpression()) {
@@ -1279,7 +1302,7 @@
       generateStatements(info.thenGraph);
       preVisitedBlocks++;
       endThen(node);
-      if (node.hasElse) {
+      if (node.hasElse && hasCodeUntil(node.elseBlock, node.joinBlock)) {
         startElse(node);
         assert(node.elseBlock === dominated[1]);
         generateStatements(info.elseGraph);