Better detection of logical and/or inlining opportunities.

lib/compiler/implementation/ssa/codegen_helpers.dart

Index: lib/compiler/implementation/ssa/codegen_helpers.dart
diff --git a/lib/compiler/implementation/ssa/codegen_helpers.dart b/lib/compiler/implementation/ssa/codegen_helpers.dart
index 41dadc6ba6e9a0ff046711c3cb12ae3943ace338..a7f74bdd44ae44a71207664f5f7ddcc3fceaff1d 100644
--- a/lib/compiler/implementation/ssa/codegen_helpers.dart
+++ b/lib/compiler/implementation/ssa/codegen_helpers.dart
@@ -181,6 +181,7 @@ class SsaConditionMerger extends HGraphVisitor {
     if (instruction is HPhi && !logicalOperations.containsKey(instruction)) {
       return false;
     }
+    HInstruction previous;

[ngeoffray, 2012/04/19 12:52:24]

Unused variable?

[Lasse Reichstein Nielsen, 2012/04/19 13:13:07]

Yes, removed.

     while (instruction.previous != null) {
       instruction = instruction.previous;
       if (!generateAtUseSite.contains(instruction)) {
@@ -188,33 +189,68 @@ class SsaConditionMerger extends HGraphVisitor {
       }
     }
     HBasicBlock block = instruction.block;
-    if (!block.phis.isEmpty()) return false;
+    if (!block.phis.isEmpty()) {
+      if (!generateAtUseSite.contains(instruction)) {

[ngeoffray, 2012/04/19 12:52:24]

So this is the very first instruction of the block, right? Maybe add a comment.

[Lasse Reichstein Nielsen, 2012/04/19 13:13:07]

Done.
Also took care of the case when [instruction] started out as an HPhi.

+        return false;
+      }
+      instruction = block.phis.last;
+      if (block.phis.first !== instruction) {

[ngeoffray, 2012/04/19 12:52:24]

block.phis.length != 1 reads better

[Lasse Reichstein Nielsen, 2012/04/19 13:13:07]

But runs worse. [block.phis] is a [HInstructionList] and doesn't have a length
property.

+        return false;
+      }
+    }
     if (instruction is HPhi && logicalOperations.containsKey(instruction)) {
       return isExpression(instruction.inputs[0], limit);
     }
-    return block.predecessors.length == 1 && block.predecessors[0] == limit;
+    if (block.predecessors.length !== 1) {
+      return false;
+    }
+    HBasicBlock previousBlock = block.predecessors[0];
+    if (previousBlock === limit) return true;
+    if (previousBlock.successors.length !== 1 ||
+        previousBlock.last is! HGoto) {

[ngeoffray, 2012/04/19 12:52:24]

Should that check be isBlockSinglePredecessor instead?

[Lasse Reichstein Nielsen, 2012/04/19 13:13:07]

It's more, since it also checks that the HControlFlow is an HGoto (probably
redundant).
Alas, I don't have access to isBlockSinglePredecessor here, it's in another
class.

+      return false;
+    }
+    return isExpression(previousBlock.last, limit);
   }
 
   void replaceWithLogicalOperator(HPhi phi, String type) {
     if (canGenerateAtUseSite(phi)) generateAtUseSite.add(phi);
     logicalOperations[phi] = type;
+    // If the phi corresponds to logical control flow, mark the
+    // control-flow instructions as generate-at-use-site.
+    generateAtUseSite.add(phi.block.predecessors[0].last);
+    generateAtUseSite.add(phi.block.predecessors[1].last);
+    // If the first input is only used as branch condition and result, it too
+    // can be generate-at-use-site.
+    if (phi.inputs[0].usedBy.length == 2) {
+      generateAtUseSite.add(phi.inputs[0]);
+    }
+    if (phi.inputs[1].usedBy.length == 1) {
+      generateAtUseSite.add(phi.inputs[1]);
+    }
   }
 
   bool canGenerateAtUseSite(HPhi phi) {
-    if (phi.usedBy.length != 1) return false;
+    if (phi.usedBy.length != 1) {
+      return false;
+    }
     assert(phi.next == null);
     HInstruction use = phi.usedBy[0];
 
     HInstruction current = phi.block.first;
     while (current != use) {
-      if (!generateAtUseSite.contains(current)) return false;
+      if (current is! HControlFlow && !generateAtUseSite.contains(current)) {

[ngeoffray, 2012/04/19 12:52:24]

Please add a comment on why you check that.

[Lasse Reichstein Nielsen, 2012/04/19 13:13:07]

Done.

+        return false;
+      }
       if (current.next != null) {
         current = current.next;
       } else if (current is HPhi) {
         current = current.block.first;
       } else {
         assert(current is HControlFlow);
-        if (current is !HGoto) return false;
+        if (current is !HGoto) {
+          return false;
+        }
         HBasicBlock nextBlock = current.block.successors[0];
         if (!nextBlock.phis.isEmpty()) {
           current = nextBlock.phis.first;
@@ -226,6 +262,23 @@ class SsaConditionMerger extends HGraphVisitor {
     return true;
   }
 
+  HInstruction previousInstruction(HInstruction instruction) {
+    if (instruction.previous != null) return instruction.previous;
+    HBasicBlock block = instruction.block;
+    if (instruction is! HPhi) {
+      if (block.phis.last != null) return block.phis.last;
+    }
+    if (block.predecessors.length == 1) {
+      HBasicBlock previousBlock = block.predecessors[0];
+      if (previousBlock.last is HGoto) {

[ngeoffray, 2012/04/19 12:52:24]

Use isBlockSinglePredecessor instead?

[Lasse Reichstein Nielsen, 2012/04/19 13:13:07]

Don't have it. I don't think it's worth copying here, and half of what it checks
is already checked here (the predecessors.length == 1 check). I'll keep the test
for HGoto, just to be safe if we ever add another single-successor control flow
instruction.

+        assert(previousBlock.successors.length == 1);
+        assert(previousBlock.successors[0] === block);
+        return previousInstruction(previousBlock.last);
+      }
+    }
+    return null;
+  }
+
   void detectLogicControlFlow(HPhi phi) {
     // Check for the most common pattern for a short-circuit logic operation:
     //   B0 b0 = ...; if (b0) goto B1 else B2 (or: if (!b0) goto B2 else B1)
@@ -234,12 +287,11 @@ class SsaConditionMerger extends HGraphVisitor {
     //   |/
     //   B2 b2 = phi(b0,b1); if(b2) ...
     // TODO(lrn): Also recognize ?:-flow?
-
     if (phi.inputs.length != 2) return;
+    HBasicBlock firstBlock = phi.block.predecessors[0];
+    HBasicBlock secondBlock = phi.block.predecessors[1];

[ngeoffray, 2012/04/19 12:52:24]

How it was written before read better for me.

[Lasse Reichstein Nielsen, 2012/04/19 13:13:07]

Reordered.

     HInstruction first = phi.inputs[0];
-    HBasicBlock firstBlock = first.block;
     HInstruction second = phi.inputs[1];
-    HBasicBlock secondBlock = second.block;
     // Check second input of phi being an expression followed by a goto.
     if (second.usedBy.length != 1) return;
     HInstruction secondNext =
@@ -251,36 +303,32 @@ class SsaConditionMerger extends HGraphVisitor {
     // Check first input of phi being followed by a (possibly negated)
     // conditional branch based on the same value.
     if (firstBlock != phi.block.dominator) return;
-    if (firstBlock.last is !HConditionalBranch) return;
-    HConditionalBranch firstBranch = firstBlock.last;
-    // Must be used both for value and for control to avoid the second branch.
-    if (first.usedBy.length != 2) return;
+    if (firstBlock.last is! HIf) return;
     if (firstBlock.successors[1] != phi.block) return;
-    HInstruction firstNext = (first is HPhi) ? firstBlock.first : first.next;
-    if (firstNext == firstBranch &&
-        firstBranch.condition == first) {
+    HIf firstBranch = firstBlock.last;
+    HInstruction condition = firstBranch.inputs[0];
+    if (condition === first) {
       replaceWithLogicalOperator(phi, "&&");
-    } else if (firstNext is HNot &&
-               firstNext.inputs[0] == first &&
-               generateAtUseSite.contains(firstNext) &&
-               firstNext.next == firstBlock.last &&
-               firstBranch.condition == firstNext) {
+    } else if (condition is HNot &&
+               condition.inputs[0] == first) {
       replaceWithLogicalOperator(phi, "||");
-    } else {
-      return;
+      // If the negation is only used by this logical operation, or only by
+      // logical operators in general, it won't need to be generated.
+      if (!generateAtUseSite.contains(condition)) {
+        for (HInstruction user in condition.usedBy) {
+          if (user is! HIf || !generateAtUseSite.contains(user)) {
+            return;
+          }
+        }
+        generateAtUseSite.add(condition);
+      }
     }
-    // Detected as logic control flow. Mark the corresponding
-    // inputs as generated at use site. These will now be generated
-    // as part of an expression.
-    generateAtUseSite.add(first);
-    generateAtUseSite.add(firstBlock.last);
-    generateAtUseSite.add(second);
-    generateAtUseSite.add(secondBlock.last);
+    return;
   }
 
   void visitBasicBlock(HBasicBlock block) {
     if (!block.phis.isEmpty() &&
-        block.phis.first == block.phis.last) {
+        block.phis.first === block.phis.last) {
       detectLogicControlFlow(block.phis.first);
     }
   }