Fix bad generate-at-use-site.

sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.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.
 
 part of ssa;
 
 /**
  * Replaces some instructions with specialized versions to make codegen easier.
  * Caches codegen information on nodes.
  */
@@ skipping 267 matching lines @@
     for (HInstruction instruction = block.last.previous;
          instruction != null;
          instruction = instruction.previous) {
       if (generateAtUseSite.contains(instruction)) {
         continue;
       }
       if (instruction.isCodeMotionInvariant()) {
         markAsGenerateAtUseSite(instruction);
         continue;
       }
-      if (instruction.isJsStatement()) {

[kasperl, 2014/03/15 07:14:48]

Can you enumerate the things we use isJsStatement for now? It is still a
confusing concept so getting rid of that (in another CL) might be beneficial.

[floitsch, 2014/03/17 10:26:14]

It informs `generateAtUseSite` that the node must not be used in an expression
context.

I will look (in a separate CL) why store-nodes are marked as `isJsStatement`.

-        expectedInputs.clear();
-      }
       if (instruction.isPure()) {
         if (pureInputs.contains(instruction)) {
           tryGenerateAtUseSite(instruction);
         } else {
           // If the input is not in the [pureInputs] set, it has not
-          // been visited.
+          // been visited or should not be generated at use-site. The most
+          // likely reason for the latter, is that the instruction is used
+          // in more than one location.
+          // We must either clear the expectedInputs, or move the pure
+          // instruction's inputs in front of the existing ones.
+          // Example:
+          //   t1 = foo();  // side-effect.
+          //   t2 = bar();  // side-effect.
+          //   t3 = pure(t2);    // used more than once.
+          //   f(t1, t3);   // expected inputs of 'f': t1.
+          //   use(t3);
+          //
+          // If we don't clear the expected inputs we end up in a situation
+          // where pure pushes "t2" on top of "t1" leading to:
+          //
+          //  t3 = pure(bar());
+          //  f(foo(), t3);
+          //  use(t3);
+          //
+          // If we clear the expected-inputs list we have the correct
+          // output:
+          //   t1 = foo();
+          //   t3 = pure(bar());
+          //   f(t1, t3);
+          //   use(t3);
+          //
+          // Clearing is, however, not optimal.
+          // Example:
+          //   t1 = foo();  // t1 is now used by `pure`.
+          //   t2 = bar();  // t2 is now used by `f`.
+          //   t3 = pure(t1);
+          //   f(t2, t3);
+          //   use(t3);
+          //
+          // If we clear the expected-inputs we can't generate-at-use any of
+          // the instructions.
+          //
+          // The optimal solution is to store move the inputs of 'pure' in

[kasperl, 2014/03/15 07:10:56]

store move -> move

[floitsch, 2014/03/17 10:26:14]

Done.

+          // front of the expectedInputs list. This makes sense, since we
+          // 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 would end up with:
+          //   t3 = pure(foo();
+          //   f(bar(), t3);
+          //   use(t3);
+          //
+          // TODO(floitsch): should we keep existing inputs and move new inputs

[kasperl, 2014/03/15 07:10:56]

File a bug for this?

[floitsch, 2014/03/17 10:26:14]

Just implemented the optimization. -> removed the TODO.

+          // to the front of the list?
+          expectedInputs.clear();
           instruction.accept(this);
         }
       } else {
         if (findInInputsAndPopNonMatching(instruction)) {
           // The current instruction is the next non-trivial
           // expected input.
           tryGenerateAtUseSite(instruction);
         } else {
           assert(expectedInputs.isEmpty);
         }
@@ skipping 177 matching lines @@
     }
 
     // If [thenInput] is defined in the first predecessor, then it is only used
     // by [phi] and can be generated at use site.
     if (identical(thenInput.block, end.predecessors[0])) {
       assert(thenInput.usedBy.length == 1);
       markAsGenerateAtUseSite(thenInput);
     }
   }
 }