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

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

Fix by treating some HLocalGet instructions as pure and others as
impure.  Simply making HLocalGet impure caused a lot of code quality
regressions with the single-assignment exception and stack-trace
variables.

R=kmillikin@google.com, sigmund@google.com

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

Reverted: https://github.com/dart-lang/sdk/commit/5fd51e63400c9e201ced8835c1d2c3c7924be975

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

[sra1, 2016-04-22 18:31:31]

sra@google.com changed reviewers:
+ kmillikin@google.com, sigmund@google.com

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

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

[Florian Schneider, 2016-04-22 20:39:09]

dbc:

https://codereview.chromium.org/1914623002/diff/1/pkg/compiler/lib/src/ssa/co...
File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/1914623002/diff/1/pkg/compiler/lib/src/ssa/co...
pkg/compiler/lib/src/ssa/codegen_helpers.dart:389: if (instruction is HLocalGet)
return !isAssignedLocal(instruction.local);
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]

https://codereview.chromium.org/1914623002/diff/1/pkg/compiler/lib/src/ssa/co...
File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/1914623002/diff/1/pkg/compiler/lib/src/ssa/co...
pkg/compiler/lib/src/ssa/codegen_helpers.dart:389: if (instruction is HLocalGet)
return !isAssignedLocal(instruction.local);
On 2016/04/22 20:39:08, Florian Schneider wrote:
> 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?

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.

[Kevin Millikin (Google), 2016-04-25 07:58:53]

LGTM.

[Siggi Cherem (dart-lang), 2016-04-25 15:13:12]

lgtm

[sra1, 2016-04-25 17:20:37]

Description was changed from

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

Fix by treating some HLocalGet instructions as pure and others as
impure.  Simply making HLocalGet impure caused a lot of code quality
regressions with the single-assignment exception and stack-trace
variables.
==========

to

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

Fix by treating some HLocalGet instructions as pure and others as
impure.  Simply making HLocalGet impure caused a lot of code quality
regressions with the single-assignment exception and stack-trace
variables.

R=kmillikin@google.com, sigmund@google.com

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

[sra1, 2016-04-25 17:20:39]

Committed patchset #3 (id:40001) manually as
e9b06a9cd7e41c80b1a60cb20ea45f21b46ac40f (presubmit successful).

[sra1, 2016-04-25 18:55:53]

Description was changed from

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

Fix by treating some HLocalGet instructions as pure and others as
impure.  Simply making HLocalGet impure caused a lot of code quality
regressions with the single-assignment exception and stack-trace
variables.

R=kmillikin@google.com, sigmund@google.com

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

to

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

Fix by treating some HLocalGet instructions as pure and others as
impure.  Simply making HLocalGet impure caused a lot of code quality
regressions with the single-assignment exception and stack-trace
variables.

R=kmillikin@google.com, sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/5fd51e63400c9e201ced8835c1d2c3c7924be975
==========

[sra1, 2016-04-25 23:29:15]

Patchset #5 (id:80001) has been deleted

[sra1, 2016-04-26 22:26:49]

Description was changed from

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

Fix by treating some HLocalGet instructions as pure and others as
impure.  Simply making HLocalGet impure caused a lot of code quality
regressions with the single-assignment exception and stack-trace
variables.

R=kmillikin@google.com, sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/5fd51e63400c9e201ced8835c1d2c3c7924be975
==========

to

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

Fix by treating some HLocalGet instructions as pure and others as
impure.  Simply making HLocalGet impure caused a lot of code quality
regressions with the single-assignment exception and stack-trace
variables.

R=kmillikin@google.com, sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/5fd51e63400c9e201ced8835c1d2c3c7924be975

Committed:
https://github.com/dart-lang/sdk/commit/7684794b8bba015d9b884cde6c8bcd62c909d069
==========

[sra1, 2016-04-26 22:26:51]

Committed patchset #5 (id:100001) manually as
7684794b8bba015d9b884cde6c8bcd62c909d069 (presubmit successful).