Avoid inserting new temporaries because of HTypeConversion nodes.

Avoid inserting new temporaries because of HTypeConversion nodes.
Committed: https://code.google.com/p/dart/source/detail?r=7421

[ngeoffray, 2012-05-08 11:17:51]

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:939: HInstruction get input() =>
null;
I could instead add a common super class for instructions that return their
input. Your call.

[Lasse Reichstein Nielsen, 2012-05-08 12:39:08]

LGTM

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:522: if
(instruction.returnsInput()) {
Use single if with '&&'.

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:574: } else if
(isGenerateAtUseSite(input) && !input.isCodeMotionInvariant()) {
Please explain the use of isCodeMotionInvariant.

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:589: // Otherwise we just use
[input].
Please give example of output here too.
What happens to argument? Is it not generated at all?

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1139: isGeneratingExpression() ?
addExpressionSeparator() : addIndentation();
Use if. Please don't (try) to be clever with the conditional operator. It hurts.
If it's too long, it seems like something that can be extracted into a reusable
method.

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:2079: HInstruction input =
node.inputs[0];
node.input?
Why extract input here, when it's only used once on two separate branches later?

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/nodes.dart:938: bool returnsInput() => false;
returnsSingleInput?

Isn't our behavior here a little odd? We insert instructions that are guaranteed
to return their input - so we could just as well have them return no input
immediately, and reuse the original input later.
For some reason we don't do that here, but have a later step that does the
transformation. Why?

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/nodes.dart:939: HInstruction get input() =>
null;
What's the "input" getter for? Add comment, especially when adding an,
effectively, abstract method where the user can't even look at the code.

http://codereview.chromium.org/10383062/diff/2001/lib/compiler/implementation...
lib/compiler/implementation/ssa/nodes.dart:981: 
Could you put the new methods in a somewhat consistent position inside the
classes. At least adjacent, as they were in the superclass.

[floitsch, 2012-05-08 13:21:41]

DBC.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:525: // but is not a trivial code
motion invariant instruction (eg
"like for". There is enough space.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:533: // In checked mode does not
need a new variable:
In checked mode no new variable is needed:

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:547: needsVar =
isGenerateAtUseSite(input) && !input.isCodeMotionInvariant();
If I understand correctly it is crucial that 'define' and 'use' agree on when to
use a var, and when not.
Therefore move the check into a separate function.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:939: HInstruction get input() =>
null;
On 2012/05/08 11:17:51, ngeoffray wrote:
> I could instead add a common super class for instructions that return their
> input. Your call.

I would prefer a common superclass or alternatively a common interface.

[ngeoffray, 2012-05-08 16:09:12]

Thank you for the comments!

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:522: if
(instruction.returnsInput()) {
On 2012/05/08 12:39:08, Lasse Reichstein Nielsen wrote:
> Use single if with '&&'.

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:525: // but is not a trivial code
motion invariant instruction (eg
On 2012/05/08 13:21:42, floitsch wrote:
> "like for". There is enough space.

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:533: // In checked mode does not
need a new variable:
On 2012/05/08 13:21:42, floitsch wrote:
> In checked mode no new variable is needed:

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:547: needsVar =
isGenerateAtUseSite(input) && !input.isCodeMotionInvariant();
On 2012/05/08 13:21:42, floitsch wrote:
> If I understand correctly it is crucial that 'define' and 'use' agree on when
to
> use a var, and when not.
> Therefore move the check into a separate function.

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:574: } else if
(isGenerateAtUseSite(input) && !input.isCodeMotionInvariant()) {
On 2012/05/08 12:39:08, Lasse Reichstein Nielsen wrote:
> Please explain the use of isCodeMotionInvariant.

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:589: // Otherwise we just use
[input].
On 2012/05/08 12:39:08, Lasse Reichstein Nielsen wrote:
> Please give example of output here too.
> What happens to argument? Is it not generated at all?

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1139: isGeneratingExpression() ?
addExpressionSeparator() : addIndentation();
On 2012/05/08 12:39:08, Lasse Reichstein Nielsen wrote:
> Use if. Please don't (try) to be clever with the conditional operator. It
hurts.
> If it's too long, it seems like something that can be extracted into a
reusable
> method.

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:2079: HInstruction input =
node.inputs[0];
On 2012/05/08 12:39:08, Lasse Reichstein Nielsen wrote:
> node.input?
> Why extract input here, when it's only used once on two separate branches
later?

Done.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:938: bool returnsInput() => false;
On 2012/05/08 12:39:08, Lasse Reichstein Nielsen wrote:
> returnsSingleInput?
> 
> Isn't our behavior here a little odd? We insert instructions that are
guaranteed
> to return their input - so we could just as well have them return no input
> immediately, and reuse the original input later.
> For some reason we don't do that here, but have a later step that does the
> transformation. Why?

As discussed, to get the dependencies right. The checks need to be before the
uses.

https://chromiumcodereview.appspot.com/10383062/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:939: HInstruction get input() =>
null;
On 2012/05/08 13:21:42, floitsch wrote:
> On 2012/05/08 11:17:51, ngeoffray wrote:
> > I could instead add a common super class for instructions that return their
> > input. Your call.
> 
> I would prefer a common superclass or alternatively a common interface.

I'm now using HCheck as the superclass of all these classes. So I removed
returnsInput and the input getter.