Refactor constant part.

Refactor constant part.

The Ssa backend now uses the CompileTimeConstantHandler's Constants.

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

[kasperl, 2012-03-05 13:53:16]

LGTM!

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:23: bool isTrue() {
This smells like you should have two separate BoolConstants: TrueConstant and
FalseConstant.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:44: Constant binaryFold(String op, Constant
other) {
It sort of rubs me the wrong way to use a string for the operation here.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:58: // the VM.
Did you file this as a bug? Update comment with bug number.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:1300: // See if we can constant-fold right away. This
avoids rewrites later on.
Nice. I wonder if there's anyway of avoiding to create the "wrong" HConstant in
the first place? It would require you to not visit the node.receiver and to be
able to get a Constant from the syntax tree somehow.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:1320: void visitBinary(HInstruction left, Operator op,
HInstruction right) {
Maybe think about if it would be possible to constant fold in the caller of this
method too (like you do for unary sends).

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:644: CompileTimeConstantHandler handler =
compiler.compileTimeConstantHandler;
Maybe compiler.compileTimeConstantHandler should be renamed? compiler.constants?

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:151: HType mapConstantTypeToSsaType(Constant constant) {
static?

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:921: bool isNullConstant() => false;
I know it doesn't read as well, but I think I'd prefer these to be
isConstantNull, etc. Somehow that just seems more consistent to me.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:1188: return graph.addNewConstant(new
IntConstant(string.length));
Maybe keeping a few helpers around (graph.addNewConstantInt) for these cases
wouldn't hurt?

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:1382: String operationAsString() => "+";
These operationAsString seem a bit broken. I wonder how much work it would be to
introduce const objects for operations?

[floitsch, 2012-03-05 15:43:24]

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:23: bool isTrue() {
On 2012/03/05 13:53:16, kasperl wrote:
> This smells like you should have two separate BoolConstants: TrueConstant and
> FalseConstant.

I will do that when I introduce caching for the common constants.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:44: Constant binaryFold(String op, Constant
other) {
On 2012/03/05 13:53:16, kasperl wrote:
> It sort of rubs me the wrong way to use a string for the operation here.

will fix in another CL.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:58: // the VM.
On 2012/03/05 13:53:16, kasperl wrote:
> Did you file this as a bug? Update comment with bug number.

bug is gone.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:1300: // See if we can constant-fold right away. This
avoids rewrites later on.
On 2012/03/05 13:53:16, kasperl wrote:
> Nice. I wonder if there's anyway of avoiding to create the "wrong" HConstant
in
> the first place? It would require you to not visit the node.receiver and to be
> able to get a Constant from the syntax tree somehow.

We could, but I'm not sure it wins us that much. Also note that we avoid more
than just -<number>:
var x = 99;
var y = -x;  will correctly be folded here.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:1320: void visitBinary(HInstruction left, Operator op,
HInstruction right) {
On 2012/03/05 13:53:16, kasperl wrote:
> Maybe think about if it would be possible to constant fold in the caller of
this
> method too (like you do for unary sends).

Of course I thought about it :)
Currently it is kind of annoying with the =,++,... mixed in. But I will continue
thinking about it.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:644: CompileTimeConstantHandler handler =
compiler.compileTimeConstantHandler;
On 2012/03/05 13:53:16, kasperl wrote:
> Maybe compiler.compileTimeConstantHandler should be renamed?
compiler.constants?

renamed to constantHandler.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:151: HType mapConstantTypeToSsaType(Constant constant) {
On 2012/03/05 13:53:16, kasperl wrote:
> static?

Done.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:921: bool isNullConstant() => false;
On 2012/03/05 13:53:16, kasperl wrote:
> I know it doesn't read as well, but I think I'd prefer these to be
> isConstantNull, etc. Somehow that just seems more consistent to me.

Done.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:1188: return graph.addNewConstant(new
IntConstant(string.length));
On 2012/03/05 13:53:16, kasperl wrote:
> Maybe keeping a few helpers around (graph.addNewConstantInt) for these cases
> wouldn't hurt?

Done.

https://chromiumcodereview.appspot.com/9595017/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:1382: String operationAsString() => "+";
On 2012/03/05 13:53:16, kasperl wrote:
> These operationAsString seem a bit broken. I wonder how much work it would be
to
> introduce const objects for operations?

will think about it in another CL.