Avoid generating empty else.

lib/compiler/implementation/ssa/codegen.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 class SsaCodeGeneratorTask extends CompilerTask {
   final JavaScriptBackend backend;
   SsaCodeGeneratorTask(JavaScriptBackend backend)
       : this.backend = backend,
         super(backend.compiler);
   String get name() => 'SSA code generator';
@@ skipping 531 matching lines @@
       }
     } else {
       generateStatements(info.condition);
       addIndented("if (");
       use(condition, JSPrecedence.EXPRESSION_PRECEDENCE);
       buffer.add(") {\n");
       indent++;
       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.

         indent--;
         addIndented("}");
       }
       buffer.add("\n");
     }
     return true;
   }
 
   bool visitSequenceInfo(HStatementSequenceInformation info) {
     return false;
@@ skipping 678 matching lines @@
     }
   }
 
   visitTry(HTry node) {
     // We should never get here. Try/catch/finally is always handled using block
     // information in [visitTryInfo], or not at all, in the case of the bailout
     // generator.
     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()) {
         use(node.inputs[0], JSPrecedence.EXPRESSION_PRECEDENCE);
       }
       return;
     }
     HInstruction condition = node.inputs[0];
     int preVisitedBlocks = 0;
     List<HBasicBlock> dominated = node.block.dominatedBlocks;
     HIfBlockInformation info = node.blockInformation.body;
     if (condition.isConstant()) {
       HConstant constant = condition;
       if (constant.constant.isTrue()) {
         generateStatements(info.thenGraph);
       } else if (node.hasElse) {
         generateStatements(info.elseGraph);
       }
       // We ignore the other branch, even if it isn't visited.
       preVisitedBlocks = node.hasElse ? 2 : 1;
     } else {
       startIf(node);
       assert(!isGenerateAtUseSite(node));
       startThen(node);
       assert(node.thenBlock === dominated[0]);
       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);
         preVisitedBlocks++;
         endElse(node);
       }
       endIf(node);
     }
     HBasicBlock joinBlock = node.joinBlock;
     if (joinBlock !== null && joinBlock.dominator !== node.block) {
@@ skipping 1176 matching lines @@
       startBailoutSwitch();
     }
   }
 
   void endLabeledBlock(HLabeledBlockInformation labeledBlockInfo) {
     if (labeledBlockInfo.body.start.hasGuards()) {
       endBailoutSwitch();
     }
   }
 }