Replaces Location::ToStackSlotAddress() with Location::ToStackSlotOffset()

Replaces Location::ToStackSlotAddress() with Location::ToStackSlotOffset()

This way, Location no longer mentions the architecture specific Address class.

R=regis@google.com

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

[Florian Schneider, 2013-07-17 07:58:01]

On 2013/07/17 00:22:02, zra wrote:

DBC:

Why is this refactoring necessary? I find it more clumsy to write
Address::StackSlotAddress(destination) instead of
destination.ToStackSlotAddress().

[zra, 2013-07-17 15:28:13]

On 2013/07/17 07:58:01, Florian Schneider wrote:
> On 2013/07/17 00:22:02, zra wrote:
> 
> DBC:
> 
> Why is this refactoring necessary? I find it more clumsy to write
> Address::StackSlotAddress(destination) instead of
> destination.ToStackSlotAddress().

In general, architecture-generic code shouldn't mention architecture-specific
things like Address. I am doing the refactoring now because I'll need to
manipulate the offset when loading/storing ARM SIMD values. ToStackSlotAddress
showed up in so few places that hopefully a little extra typing isn't a bit
deal.

[srdjan, 2013-07-17 16:13:04]

DBC

https://codereview.chromium.org/19464002/diff/11001/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/19464002/diff/11001/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:15: #include "vm/locations.h"
I do not think that assembler should depend on optimizing's compiler structure
like Location. IMHO, the function ToStackSlotAddress() should be moved into
machine dependent source file.

[regis, 2013-07-17 16:35:18]

Zach,

Thanks for addressing [sic] my todo of removing all occurences of Address in the
architecture independent portion of the vm.

I agree with Florian, though, that it is more cumbersome to use.

I do not like the fact that location.h (high level abstraction) is now included
in the assemblers (rather low level stuff).

I see that Srdjan thinks alike.

See my suggestion below.

-- Regis

https://codereview.chromium.org/19464002/diff/11001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/19464002/diff/11001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:1654: // Do not implement or use this
function.
The 4 functions below should also be part of a similar refactoring. You could
add a TODO here.

https://codereview.chromium.org/19464002/diff/11001/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://codereview.chromium.org/19464002/diff/11001/runtime/vm/locations.h#ne...
runtime/vm/locations.h:285: intptr_t ToStackSlotOffset() const;
You could add this new function, while leaving ToStackSlotAddress() untouched.
The IA32 and X64 code generator could continue using the old one, while ARM and
MIPS could use the new one when the offset may be out of range (LoadFromOffset
and StoreToOffset are used instead).

[zra, 2013-07-17 18:30:45]

I've applied the suggestions from Regis. Please have another look.

[regis, 2013-07-17 18:46:45]

LGTM

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:740: // Address::Equals() on ARM/MIPS.
I think you can do what the TODO says and then remove the TODO.

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:1792: __ StoreToOffset(kWord, TMP, FP,
source_offset);
How about implementing a variant of Exchange that takes stack offsets?

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:779: if (!src.Equals(dest)) {
I guess Equals can remain on Intel, or you compare the offsets instead.

[zra, 2013-07-17 20:40:11]

Thanks!

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:740: // Address::Equals() on ARM/MIPS.
On 2013/07/17 18:46:45, regis wrote:
> I think you can do what the TODO says and then remove the TODO.

Done.

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:1792: __ StoreToOffset(kWord, TMP, FP,
source_offset);
On 2013/07/17 18:46:45, regis wrote:
> How about implementing a variant of Exchange that takes stack offsets?

Done.

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/19464002/diff/20001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:779: if (!src.Equals(dest)) {
On 2013/07/17 18:46:45, regis wrote:
> I guess Equals can remain on Intel, or you compare the offsets instead.

I have left Equals here, but I think this is the only place it is used.

[zra, 2013-07-17 20:54:42]

Committed patchset #4 manually as r25122 (presubmit successful).

[Florian Schneider, 2013-07-18 10:39:25]

https://codereview.chromium.org/19464002/diff/33001/runtime/vm/locations.cc
File runtime/vm/locations.cc (right):

https://codereview.chromium.org/19464002/diff/33001/runtime/vm/locations.cc#n...
runtime/vm/locations.cc:90: Address Location::ToStackSlotAddress() const {
return Address(FPREG, ToStackSlotOffset());