Fix vm code base so that it can be built for --arch=simarm (no snapshot yet).

Fix vm code base so that it can be built for --arch=simarm (no snapshot yet).
Committed: https://code.google.com/p/dart/source/detail?r=17246

[Ivan Posva, 2013-01-16 01:17:56]

First set of comments.

-Ivan

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

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:63: bool HasNear() const { return false; }
Does ARM have near branches?

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:93: static bool sse2_supported() { return false; }
This makes no sense for ARM, it needs to be taken out of the shared code.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_ia32.h#n...
runtime/vm/assembler_ia32.h:633: static const char* FpuRegisterName(XmmRegister
reg);
This looks very asymmetric. Either it should take a FpuRegister as a parameter
or be called XmmRegisterName.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_x64.h#ne...
runtime/vm/assembler_x64.h:653: static const char* FpuRegisterName(XmmRegister
reg);
ditto

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1294: void
FlowGraphCompiler::LoadDoubleOrSmiToFpu(XmmRegister result,
XmmRegister -> FpuRegister to match the header.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_x64.cc:1298: void
FlowGraphCompiler::LoadDoubleOrSmiToFpu(XmmRegister result,
ditto

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_x64.h (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_x64.h:69: void LoadDoubleOrSmiToFpu(XmmRegister
result,
ditto

[regis, 2013-01-16 01:55:07]

First set of comments addressed.

Thanks,
Regis

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

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:63: bool HasNear() const { return false; }
On 2013/01/16 01:17:56, Ivan Posva wrote:
> Does ARM have near branches?

No, but FlowGraphCompiler::~FlowGraphCompiler() is doing some architecture
dependent label checking. I duplicated it for each architecture and removed
HasNear() here.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:93: static bool sse2_supported() { return false; }
On 2013/01/16 01:17:56, Ivan Posva wrote:
> This makes no sense for ARM, it needs to be taken out of the shared code.

This is used by the flow_graph_optimizer, which I did not want to tackle in this
cl.

I have removed sse2_supported() and sse4_1_supported() here and added a TODO
with an #if defined in flow_graph_optimizer.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_ia32.h#n...
runtime/vm/assembler_ia32.h:633: static const char* FpuRegisterName(XmmRegister
reg);
On 2013/01/16 01:17:56, Ivan Posva wrote:
> This looks very asymmetric. Either it should take a FpuRegister as a parameter
> or be called XmmRegisterName.

Renamed parameter type.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/assembler_x64.h#ne...
runtime/vm/assembler_x64.h:653: static const char* FpuRegisterName(XmmRegister
reg);
On 2013/01/16 01:17:56, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1294: void
FlowGraphCompiler::LoadDoubleOrSmiToFpu(XmmRegister result,
On 2013/01/16 01:17:56, Ivan Posva wrote:
> XmmRegister -> FpuRegister to match the header.

Done.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_x64.cc:1298: void
FlowGraphCompiler::LoadDoubleOrSmiToFpu(XmmRegister result,
On 2013/01/16 01:17:56, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_x64.h (right):

https://codereview.chromium.org/11956004/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_x64.h:69: void LoadDoubleOrSmiToFpu(XmmRegister
result,
On 2013/01/16 01:17:56, Ivan Posva wrote:
> ditto

Done.

[srdjan, 2013-01-16 19:06:30]

LGTM with comments

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

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:19: // Operand can be sub classed (e.g: Address).
s/sub classes/ subclassed/

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:64: 
delete line

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

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:1417: }
Put SortICDataByCount into shared FlowGraphCompiler

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1314: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
Instead of ifdef, use a shared e.g.,  SuportsDoubleTruncRound.

[regis, 2013-01-16 23:14:42]

Thanks. Please, have another look.

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

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:19: // Operand can be sub classed (e.g: Address).
On 2013/01/16 19:06:30, srdjan wrote:
> s/sub classes/ subclassed/

Done.

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:64: 
On 2013/01/16 19:06:30, srdjan wrote:
> delete line

Done.

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

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:1417: }
On 2013/01/16 19:06:30, srdjan wrote:
> Put SortICDataByCount into shared FlowGraphCompiler

Added TODO(regis): Make static member, move to shared FlowGraphCompiler, and
merge headers.

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1314: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
On 2013/01/16 19:06:30, srdjan wrote:
> Instead of ifdef, use a shared e.g.,  SuportsDoubleTruncRound.

As soon as we implement the arm version, there will be many more differences.
The cleaner solution is to split this file into architecture dependent files, as
noted by the TODO. But if you think it will be possible to keep this file
architecture independent, I will revisit.

[srdjan, 2013-01-16 23:57:56]

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1314: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
On 2013/01/16 23:14:42, regis wrote:
> On 2013/01/16 19:06:30, srdjan wrote:
> > Instead of ifdef, use a shared e.g.,  SuportsDoubleTruncRound.
> 
> As soon as we implement the arm version, there will be many more differences.
> The cleaner solution is to split this file into architecture dependent files,
as
> noted by the TODO. But if you think it will be possible to keep this file
> architecture independent, I will revisit.

I think that the optimizer will remain machine independent.

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

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:29: Operand() { }  // Needed by subclass Address.
DISALLOW_ALLOCATION ?

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.h:33: DISALLOW_COPY_AND_ASSIGN(Immediate);
Need to be fixed as discussed (Mac build complains).

[regis, 2013-01-17 01:02:07]

Please, take another look and try to build on Mac.
Thanks.

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11956004/diff/11002/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1314: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
On 2013/01/16 23:57:56, srdjan wrote:
> On 2013/01/16 23:14:42, regis wrote:
> > On 2013/01/16 19:06:30, srdjan wrote:
> > > Instead of ifdef, use a shared e.g.,  SuportsDoubleTruncRound.
> > 
> > As soon as we implement the arm version, there will be many more
differences.
> > The cleaner solution is to split this file into architecture dependent
files,
> as
> > noted by the TODO. But if you think it will be possible to keep this file
> > architecture independent, I will revisit.
> 
> I think that the optimizer will remain machine independent.

Alright. I have added a double_truncate_round_supported() static method to keep
the same style.

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

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:29: Operand() { }  // Needed by subclass Address.
On 2013/01/16 23:57:56, srdjan wrote:
> DISALLOW_ALLOCATION ?

Done.

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.h:33: DISALLOW_COPY_AND_ASSIGN(Immediate);
On 2013/01/16 23:57:56, srdjan wrote:
> Need to be fixed as discussed (Mac build complains).

Done.

[Kevin Millikin (Google), 2013-02-04 11:15:03]

DBC.

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/allocation.h
File runtime/vm/allocation.h (right):

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/allocation.h#n...
runtime/vm/allocation.h:27: DISALLOW_COPY_AND_ASSIGN(ValueObject);
It seems strange and not useful that ValueObject is the base class of objects
that *can't* be passed by value.

https://codereview.chromium.org/11956004/diff/19002/runtime/vm/allocation.h
File runtime/vm/allocation.h (right):

https://codereview.chromium.org/11956004/diff/19002/runtime/vm/allocation.h#n...
runtime/vm/allocation.h:1: // Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
I think we decided to follow the Chromium project's guidelines and not update
copyright dates when a file was changed.

[regis, 2013-02-04 16:41:44]

Thanks Kevin.

-- Regis

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/allocation.h
File runtime/vm/allocation.h (right):

https://codereview.chromium.org/11956004/diff/10006/runtime/vm/allocation.h#n...
runtime/vm/allocation.h:27: DISALLOW_COPY_AND_ASSIGN(ValueObject);
On 2013/02/04 11:15:03, kmillikin wrote:
> It seems strange and not useful that ValueObject is the base class of objects
> that *can't* be passed by value.

Objects deriving from ValueObject *can* be passed by value, but we want them to
declare their own copy constructor and assignment operator to make it clear.
As you can see by the number of patches (you are actually commenting on an older
patch :-) there was a lot of discussions here on what is the best style. The
compiler on the Mac is also buggy and difficult to please. We may revisit this
at some time.

https://codereview.chromium.org/11956004/diff/19002/runtime/vm/allocation.h
File runtime/vm/allocation.h (right):

https://codereview.chromium.org/11956004/diff/19002/runtime/vm/allocation.h#n...
runtime/vm/allocation.h:1: // Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
On 2013/02/04 11:15:04, kmillikin wrote:
> I think we decided to follow the Chromium project's guidelines and not update
> copyright dates when a file was changed.

Yes, it was brought to my attention and I stopped doing it. But I did not go
back to reset 2013 to the old year.