Fix bad generate-at-use-site.

Fix bad generate-at-use-site.

BUG= http://dartbug.com/17483
R=kasperl@google.com

Committed: https://code.google.com/p/dart/source/detail?r=33739

[kasperl, 2014-03-15 07:10:55]

LGTM.

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:330: // The
optimal solution is to store move the inputs of 'pure' in
store move -> move

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:339: //
TODO(floitsch): should we keep existing inputs and move new inputs
File a bug for this?

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

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (left):

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:288: if
(instruction.isJsStatement()) {
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]

PTAL.

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (left):

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:288: if
(instruction.isJsStatement()) {
On 2014/03/15 07:14:48, kasperl wrote:
> 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.

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`.

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:330: // The
optimal solution is to store move the inputs of 'pure' in
On 2014/03/15 07:10:56, kasperl wrote:
> store move -> move

Done.

https://codereview.chromium.org/196223017/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:339: //
TODO(floitsch): should we keep existing inputs and move new inputs
On 2014/03/15 07:10:56, kasperl wrote:
> File a bug for this?

Just implemented the optimization. -> removed the TODO.

[kasperl, 2014-03-17 10:38:23]

Still LGTM.

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:308: //  t3 =
pure(bar());
Indent code with two spaces. Maybe remove leading newline for consistency?

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:331: // front
of the expectedInputs list. This makes sense, since we
Front is slightly misleading to me. See next comment.

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:342: // Move
the pure instruction's inputs to the front.
Front is a little bit of a weird word here. It's the back of the list, right?
Maybe use a word like top instead?

Maybe the ideal would be to change expectedInputs to be a class that wraps a
list that has stack behavior and uses stack terminology rather than list
terminology (push/pop/move to top...)? That probably fits in another CL.

[floitsch, 2014-03-17 11:43:01]

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:308: //  t3 =
pure(bar());
On 2014/03/17 10:38:24, kasperl wrote:
> Indent code with two spaces. Maybe remove leading newline for consistency?

Done.

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:331: // front
of the expectedInputs list. This makes sense, since we
On 2014/03/17 10:38:24, kasperl wrote:
> Front is slightly misleading to me. See next comment.

see next comment.

https://codereview.chromium.org/196223017/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:342: // Move
the pure instruction's inputs to the front.
On 2014/03/17 10:38:24, kasperl wrote:
> Front is a little bit of a weird word here. It's the back of the list, right?
> Maybe use a word like top instead?
> 
> Maybe the ideal would be to change expectedInputs to be a class that wraps a
> list that has stack behavior and uses stack terminology rather than list
> terminology (push/pop/move to top...)? That probably fits in another CL.

It's actually the front. Since we fill the expressions from the back, it thus
becomes at the bottom of the queue.
Considering adding a data-structure in another CL.

[floitsch, 2014-03-17 11:44:26]

Committed patchset #3 manually as r33739 (presubmit successful).