Add spill slot locations.

Add spill slot locations.

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

[Vyacheslav Egorov (Google), 2012-07-23 11:49:29]

[this is piece of register allocation infrastructure
http://codereview.chromium.org/10800037]

[Kevin Millikin (Google), 2012-07-23 12:52:30]

LGTM.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_ia32.cc:1188: __ movl(EAX, ToAddress(source));
This code would be much nicer if there were helpers as in the x64 macro
assembler.  Perhaps they belong to the ParallelMoveResolver and implemented on
all platforms.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/il_printer.cc
File runtime/vm/il_printer.cc (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/il_printer....
runtime/vm/il_printer.cc:471: f->Print(" = ");
I like something that emphasizes the direction of the move, like '<-'.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:20: // Each location is encoded as a single word: low 3
bits denote location kind,
Not entirely true: tag is variable sized (alternatively, both 001 and 101 are
used as tags for constants and the payload steals the high bit of the tag).

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:63: ASSERT((kConstant & kConstantMask) == kConstant);
I wonder why we don't have STATIC_ASSERT.  Here we really would want something
like:

STATIC_ASSERT((kConstant & kConstantMask) == kConstant);
ASSERT((kind() == kConstant) || ((value_ & kConstantMask) != kConstant));

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:138: return static_cast<uword>(payload());
payload() is a uword already.

[Vyacheslav Egorov (Google), 2012-07-23 14:06:10]

Thanks for the review. Landing.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_ia32.cc:1188: __ movl(EAX, ToAddress(source));
On 2012/07/23 12:52:30, kmillikin wrote:
> This code would be much nicer if there were helpers as in the x64 macro
> assembler.  Perhaps they belong to the ParallelMoveResolver and implemented on
> all platforms.

Done. But it looks a bit ugly because on x64 only assembler can use TMP so
helpers in Resolver just delegate to assembler helpers.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/il_printer.cc
File runtime/vm/il_printer.cc (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/il_printer....
runtime/vm/il_printer.cc:471: f->Print(" = ");
On 2012/07/23 12:52:30, kmillikin wrote:
> I like something that emphasizes the direction of the move, like '<-'.

Done.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:20: // Each location is encoded as a single word: low 3
bits denote location kind,
On 2012/07/23 12:52:30, kmillikin wrote:
> Not entirely true: tag is variable sized (alternatively, both 001 and 101 are
> used as tags for constants and the payload steals the high bit of the tag).

Rephrased to make it more clear.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:63: ASSERT((kConstant & kConstantMask) == kConstant);
On 2012/07/23 12:52:30, kmillikin wrote:
> I wonder why we don't have STATIC_ASSERT.  Here we really would want something
> like:
> 
> STATIC_ASSERT((kConstant & kConstantMask) == kConstant);
> ASSERT((kind() == kConstant) || ((value_ & kConstantMask) != kConstant));

(kind() == kConstant) <- this is incorrect.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:138: return static_cast<uword>(payload());
On 2012/07/23 12:52:30, kmillikin wrote:
> payload() is a uword already.

I guess I wanted cast to intptr_t

[srdjan, 2012-07-23 16:18:59]

LGTM
The comments are just for information, I will address/implement them in a next
CL.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/assembler_x...
File runtime/vm/assembler_x64.cc (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/assembler_x...
runtime/vm/assembler_x64.cc:1514: movq(dst, TMP);
The code can be better for Smi.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/assembler_x...
File runtime/vm/assembler_x64.h (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/assembler_x...
runtime/vm/assembler_x64.h:392: void xorq(const Address& dst, Register src);
New assembly functions need test.

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_ia32.cc:1112: static Address ToAddress(Location
loc) {
The name is to generic: s/ToAddress/ToSpillAddress/

https://chromiumcodereview.appspot.com/10805053/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_ia32.cc:1189: __ movl(ECX,
ToAddress(destination));
Please upload the latest CL once comments are addressed.