Fix for issue 26243 - illegal motion of assignments in instruction merging

pkg/compiler/lib/src/ssa/codegen_helpers.dart

Index: pkg/compiler/lib/src/ssa/codegen_helpers.dart
diff --git a/pkg/compiler/lib/src/ssa/codegen_helpers.dart b/pkg/compiler/lib/src/ssa/codegen_helpers.dart
index 254e577a31f6141119a0e9d6f9c1fd77a60370a7..681056f6ce29c694a6765c1b4b8a75f95c323d1d 100644
--- a/pkg/compiler/lib/src/ssa/codegen_helpers.dart
+++ b/pkg/compiler/lib/src/ssa/codegen_helpers.dart
@@ -353,7 +353,7 @@ class SsaInstructionMerger extends HBaseVisitor {
           input is! HPhi &&
           input is! HLocalValue &&
           !input.isJsStatement()) {
-        if (input.isPure()) {
+        if (isEffectivelyPure(input)) {
           // Only consider a pure input if it is in the same loop.
           // Otherwise, we might move GVN'ed instruction back into the
           // loop.
@@ -382,6 +382,25 @@ class SsaInstructionMerger extends HBaseVisitor {
     }
   }
 
+  // Some non-pure instructions may be treated as pure. HLocalGet depends on
+  // assignments, but we can ignore the initializing assignment since it will by
+  // construction always precede a use.
+  bool isEffectivelyPure(HInstruction instruction) {
+    if (instruction is HLocalGet) return !isAssignedLocal(instruction.local);

[Florian Schneider, 2016/04/22 20:39:08]

I don't understand, but this seems to be overly conservative. Isn't the set of
assignments known for each local so that a load can never be moved above the
reaching definitions?

[sra1, 2016/04/22 21:03:55]

A conservative solution is appropriate for 1.16, especially considering that
there is a long history of problems with this pass.

This analysis is fusing small JS expressions into larger ones by moving code to
the use site without changing the order of any effects.
Many expressions are fused without moving them, they just have to execute in the
same order to preserve order of effects and dependencies.

The most conservative approach would be to treat HLocalGet like any other
effecting or effect sensitive instruction, but that produces a lot of stupid
temporaries.

It only affects code containing try-catch-finally since otherwise the function
is completely ssa converted.

+    return instruction.isPure();
+  }
+
+  bool isAssignedLocal(HLocalValue local) {
+    // [HLocalValue]s have an initializing assignment which is guaranteed to
+    // precede the use, except for [HParameterValue]s which are 'assigned' at
+    // entry.
+    int initializingAssignmentCount = (local is HParameterValue) ? 0 : 1;
+    return local.usedBy
+        .where((user) => user is HLocalSet)
+        .skip(initializingAssignmentCount)
+        .isNotEmpty;
+  }
+
   void visitInstruction(HInstruction instruction) {
     // A code motion invariant instruction is dealt before visiting it.
     assert(!instruction.isCodeMotionInvariant());
@@ -481,7 +500,7 @@ class SsaInstructionMerger extends HBaseVisitor {
     // Pop instructions from expectedInputs until instruction is found.
     // Return true if it is found, or false if not.
     bool findInInputsAndPopNonMatching(HInstruction instruction) {
-      assert(!instruction.isPure());
+      assert(!isEffectivelyPure(instruction));
       while (!expectedInputs.isEmpty) {
         HInstruction nextInput = expectedInputs.removeLast();
         assert(!generateAtUseSite.contains(nextInput));
@@ -504,7 +523,7 @@ class SsaInstructionMerger extends HBaseVisitor {
         markAsGenerateAtUseSite(instruction);
         continue;
       }
-      if (instruction.isPure()) {
+      if (isEffectivelyPure(instruction)) {
         if (pureInputs.contains(instruction)) {
           tryGenerateAtUseSite(instruction);
         } else {
@@ -550,7 +569,7 @@ class SsaInstructionMerger extends HBaseVisitor {
           // push expected-inputs from left-to right, and the `pure` function
           // invocation is "more left" (i.e. before) the first argument of `f`.
           // With that approach we end up with:
-          //   t3 = pure(foo();
+          //   t3 = pure(foo());
           //   f(bar(), t3);
           //   use(t3);
           //