dart2js cps: Do not propagate expressions into foreign code.

dart2js cps: Do not propagate expressions into foreign code.

BUG=
R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/c10bc949379065117d12bfb6aaaf104648658ee9

[asgerf, 2015-11-11 13:50:38]

asgerf@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2015-11-11 13:55:38]

lgtm

[asgerf, 2015-11-11 13:57:02]

Committed patchset #2 (id:20001) manually as
c10bc949379065117d12bfb6aaaf104648658ee9 (presubmit successful).

[sra1, 2015-11-12 02:30:08]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-11-12 02:30:09]

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:1151: //      
        definitely evaluated left-to-right.
We need to do this. The cost on code quality of not doing so is too high (adds
nearly 6k to swarm).

      get$length: function(receiver) {
        return this._html$_element.classList.length;
      },
-->
      get$length: function(receiver) {
        var v0 = this._html$_element;
        v0 = v0.classList;
        return v0.length;
      },

What would be the most convenient form of the information?

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:740: found = true;
The documentation for JS says 'never use `#` in a function body'.

There is no way to get a correct result if the capture happens in a loop, or if
the captured variable is a re-used temporary.

We should reject this code outright, and force the use of 'let' (a JS function
application) to capture the values.

I'll take a look at what is involved.

[asgerf, 2015-11-12 12:09:46]

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:1151: //      
        definitely evaluated left-to-right.
On 2015/11/12 02:30:08, sra1 wrote:
> We need to do this. The cost on code quality of not doing so is too high (adds
> nearly 6k to swarm).
> 
>       get$length: function(receiver) {
>         return this._html$_element.classList.length;
>       },
> -->
>       get$length: function(receiver) {
>         var v0 = this._html$_element;
>         v0 = v0.classList;
>         return v0.length;
>       },
> 
> What would be the most convenient form of the information?

I think we can encode the whole thing with an integer N, meaning the first N
arguments will definitely be evaluated exactly once, in left-to-right order,
without intermediate side-effects or heap dependencies, before any of the
remaining arguments are evaluated.

So for example, if the template is "foo(#,#) && bar(#,#)", N=2 because the first
two arguments are always evaluated.

The requirement about side effects and heap dependencies might be a problem. For
example, if the template was "#.length > #", and the second argument is x.pop(),
we must evaluate the pop() before getting the length, so it can't propagate. N=1
is the only safe answer there.

We could assume that the side-effect spec string of the whole template also
summarizes the side-effects in-between the arguments. So if the entire
expression has no side effects/dependencies, we can assume there are also no
side effects/dependencies separating the arguments. It won't change N=1 in the
previous example, but it saves us from doing an essentially impossible
side-effect analysis on the JS AST.

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1436833002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:740: found = true;
On 2015/11/12 02:30:09, sra1 wrote:
> The documentation for JS says 'never use `#` in a function body'.
> 
> There is no way to get a correct result if the capture happens in a loop, or
if
> the captured variable is a re-used temporary.
> 
> We should reject this code outright, and force the use of 'let' (a JS function
> application) to capture the values.
> 
> I'll take a look at what is involved.

FWIW, this change was made to fix the issue with re-used temporaries. The
register allocation will never merge captured variables with any other variables
(actually a leftover from dart2dart, where variable capture was a real thing).