Fix bad assumption of environment builder when it comes to nested loops: blocks inside a loop would…

sdk/lib/_internal/compiler/implementation/ssa/bailout.dart

Index: sdk/lib/_internal/compiler/implementation/ssa/bailout.dart
===================================================================
--- sdk/lib/_internal/compiler/implementation/ssa/bailout.dart	(revision 15084)
+++ sdk/lib/_internal/compiler/implementation/ssa/bailout.dart	(working copy)
@@ -16,12 +16,12 @@
  */
 class Environment {
   final Set<HInstruction> lives;
-  final Set<HBasicBlock> loopMarkers;
+  final List<HBasicBlock> loopMarkers;
   Environment() : lives = new Set<HInstruction>(),
-                  loopMarkers = new Set<HBasicBlock>();
+                  loopMarkers = new List<HBasicBlock>();
   Environment.from(Environment other)
     : lives = new Set<HInstruction>.from(other.lives),
-      loopMarkers = new Set<HBasicBlock>.from(other.loopMarkers);
+      loopMarkers = new List<HBasicBlock>.from(other.loopMarkers);
 
   void remove(HInstruction instruction) {
     lives.remove(instruction);
@@ -48,20 +48,13 @@
     }
   }
 
-  void addLoopMarker(HBasicBlock block) {
-    loopMarkers.add(block);
-  }
-
-  void removeLoopMarker(HBasicBlock block) {
-    loopMarkers.remove(block);
-  }
-
   void addAll(Environment other) {
     lives.addAll(other.lives);
-    loopMarkers.addAll(other.loopMarkers);
   }
 
   bool get isEmpty => lives.isEmpty && loopMarkers.isEmpty;
+
+  String toString() => '${loopMarkers.length}';

[floitsch, 2012/11/20 15:39:31]

Remove or make it at least mention "Environment".

[ngeoffray, 2012/11/20 16:40:37]

Removed.

 }
 
 
@@ -257,10 +250,12 @@
   final Map<HBailoutTarget, Environment> capturedEnvironments;
   final Map<HBasicBlock, Environment> liveInstructions;
   Environment environment;
+  List<HBasicBlock> loopMarkers;

[floitsch, 2012/11/20 15:39:31]

Add comment explaining what these contain.

[ngeoffray, 2012/11/20 16:40:37]

Done.

 
   SsaEnvironmentBuilder(Compiler this.compiler)
     : capturedEnvironments = new Map<HBailoutTarget, Environment>(),
-      liveInstructions = new Map<HBasicBlock, Environment>();
+      liveInstructions = new Map<HBasicBlock, Environment>(),
+      loopMarkers = <HBasicBlock>[];
 
 
   void visitGraph(HGraph graph) {
@@ -290,9 +285,9 @@
     // in this case {x}.
     capturedEnvironments.forEach((ignoredInstruction, env) {
       env.loopMarkers.forEach((HBasicBlock header) {
-        env.removeLoopMarker(header);
         env.addAll(liveInstructions[header]);
       });
+      env.loopMarkers.clear();
     });
   }
 
@@ -310,7 +305,8 @@
         // If we haven't computed the liveInstructions of that successor, we
         // know it must be a loop header.
         assert(successor.isLoopHeader());
-        environment.addLoopMarker(successor);
+        assert(!block.isLoopHeader());
+        loopMarkers.add(successor);
       }
 
       int index = successor.predecessors.indexOf(block);
@@ -319,6 +315,14 @@
       }
     }
 
+    // If the block is a loop header, we can remove the loop marker,

[floitsch, 2012/11/20 15:39:31]

This sounds too much like an optimization.
In fact without this if the 'loopMarkers' list would never shrink.

[ngeoffray, 2012/11/20 16:40:37]

Improved comment.

+    // because it will just recompute the loop phis.
+    if (block.isLoopHeader()) {
+      loopMarkers.removeLast();

[floitsch, 2012/11/20 15:39:31]

assert that it is the same as block?

[ngeoffray, 2012/11/20 16:40:37]

Done.

[ngeoffray, 2012/11/20 17:30:45]

Actually, I had to change the data structure to be a set, because the last may
not be the current loop header in case of labeled loops.

+    }
+
+    environment.loopMarkers.addAll(loopMarkers);
+
     // Iterate over all instructions to remove an instruction from the
     // environment and add its inputs.
     HInstruction instruction = block.last;
@@ -333,12 +337,6 @@
       environment.remove(phi);
     }
 
-    // If the block is a loop header, we can remove the loop marker,
-    // because it will just recompute the loop phis.
-    if (block.isLoopHeader()) {
-      environment.removeLoopMarker(block);
-    }
-
     // Finally save the liveInstructions of that block.
     liveInstructions[block] = environment;
   }