DBC: Adds simdbc64 target, adds arm64 arithmetic overflow logic

DBC: Adds simdbc64 target, adds arm64 arithmetic overflow logic

There were differences between linux gcc, android gcc,
MacOS clang, and 32 vs. 64-bit w.r.t the __builtin
functions for detecting arithmetic overflow. I couldn't get
them all working at the same time. Instead, I removed them,
and changed to always use the inline assembly. This works
in all the configurations above.

This change also adds a simdbc64 target for building simdbc
for 64-bit, and sets up the android targets. simdbc targets
arm, and simdbc64 targets arm64. You can build them with:

$ ./tools/build.py -m release -a simdbc{64} --os=android runtime

R=iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/3d7202bad38c09b956a3f76bf87f736bd53ebb35

[zra, 2016-04-21 22:08:39]

Description was changed from

==========
DBC: Don't use compiler intrinsics. Use inline asm for x64, arm64
==========

to

==========
DBC: Adds simdbc64 target, adds arm64 arithmetic overflow logic

There were differences between linux gcc, android gcc, MacOS clang, and 32 vs.
64-bit w.r.t the __builtin functions for detecting arithmetic overflow. I
couldn't get them all working at the same time. Instead, I removed them, and
changed to always use the inline assembly. This works in all the configurations
above.

This change also adds a simdbc64 target for building simdbc for 64-bit, and sets
up the android targets. simdbc targets arm, and simdbc64 targets arm64. You can
build them with:

$ ./tools/build.py -m release -a simdbc{64} --os=android runtime
==========

[zra, 2016-04-21 22:09:18]

Description was changed from

==========
DBC: Adds simdbc64 target, adds arm64 arithmetic overflow logic

There were differences between linux gcc, android gcc, MacOS clang, and 32 vs.
64-bit w.r.t the __builtin functions for detecting arithmetic overflow. I
couldn't get them all working at the same time. Instead, I removed them, and
changed to always use the inline assembly. This works in all the configurations
above.

This change also adds a simdbc64 target for building simdbc for 64-bit, and sets
up the android targets. simdbc targets arm, and simdbc64 targets arm64. You can
build them with:

$ ./tools/build.py -m release -a simdbc{64} --os=android runtime
==========

to

==========
DBC: Adds simdbc64 target, adds arm64 arithmetic overflow logic

There were differences between linux gcc, android gcc,
MacOS clang, and 32 vs. 64-bit w.r.t the __builtin 
functions for detecting arithmetic overflow. I couldn't get
them all working at the same time. Instead, I removed them,
and changed to always use the inline assembly. This works
in all the configurations above.

This change also adds a simdbc64 target for building simdbc
for 64-bit, and sets up the android targets. simdbc targets
arm, and simdbc64 targets arm64. You can build them with:

$ ./tools/build.py -m release -a simdbc{64} --os=android runtime
==========

[zra, 2016-04-21 22:20:42]

zra@google.com changed reviewers:
+ iposva@google.com, vegorov@google.com

[zra, 2016-04-21 22:20:42]

zra@google.com changed reviewers:
+ iposva@google.com, vegorov@google.com

[Ivan Posva, 2016-04-21 23:35:15]

-Ivan

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:349: "add %2, %1\n"
Shouldn't this be at least an addq for HOST_ARCH_X64?

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:388: #if defined(HOST_ARCH_IA32) ||
defined(HOST_ARCH_X64)
dittoq

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:429: #if defined(HOST_ARCH_IA32) ||
defined(HOST_ARCH_X64)
dittoq

[Vyacheslav Egorov (Google), 2016-04-22 06:22:04]

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (left):

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:341: #if defined(__has_builtin)
Where did this go?

Inline assembly is just a fall back, compiler intrinsics are the right thing to
rely on. 

I would expect that clang used to compile for ARM64 actually has support for
overflow intrinsics.

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:346: int32_t res = 1;
if you make this intptr_t then both ARM and ARM64 become the same code as far as
I see

[zra, 2016-04-22 19:40:50]

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (left):

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:341: #if defined(__has_builtin)
On 2016/04/22 06:22:04, Vyacheslav Egorov (Google) wrote:
> Where did this go?
> 
> Inline assembly is just a fall back, compiler intrinsics are the right thing
to
> rely on. 
> 
> I would expect that clang used to compile for ARM64 actually has support for
> overflow intrinsics.
> 

I found the problem with the intrinsics. Their use here assumes that when there
is overflow, the output is not modified. That is what the inline assembly does.
But the intrinsics may/should modify the output, even when there is overflow:

https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html.

Ivan has suggested that I keep the changes here, and add TODOs to investigate
using the intrinsics once we have something working.

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:346: int32_t res = 1;
On 2016/04/22 06:22:04, Vyacheslav Egorov (Google) wrote:
> if you make this intptr_t then both ARM and ARM64 become the same code as far
as
> I see

Done.

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:349: "add %2, %1\n"
On 2016/04/21 23:35:15, Ivan Posva wrote:
> Shouldn't this be at least an addq for HOST_ARCH_X64?

The correct ops for the types of the provided operands are generated.

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:388: #if defined(HOST_ARCH_IA32) ||
defined(HOST_ARCH_X64)
On 2016/04/21 23:35:15, Ivan Posva wrote:
> dittoq

Acknowledged.

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:429: #if defined(HOST_ARCH_IA32) ||
defined(HOST_ARCH_X64)
On 2016/04/21 23:35:15, Ivan Posva wrote:
> dittoq

Acknowledged.

[zra, 2016-04-22 21:13:27]

ptal

https://codereview.chromium.org/1904153003/diff/100001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.h (right):

https://codereview.chromium.org/1904153003/diff/100001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.h:40: static const uword kSimulatorStackUnderflowSize =
0x80;
I had to make this bigger for stack overflow tests to pass on 64-bit.

[Vyacheslav Egorov (Google), 2016-04-23 18:58:35]

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (left):

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:341: #if defined(__has_builtin)
On 2016/04/22 19:40:49, zra wrote:
> On 2016/04/22 06:22:04, Vyacheslav Egorov (Google) wrote:
> > Where did this go?
> > 
> > Inline assembly is just a fall back, compiler intrinsics are the right thing
> to
> > rely on. 
> > 
> > I would expect that clang used to compile for ARM64 actually has support for
> > overflow intrinsics.
> > 
> 
> I found the problem with the intrinsics. Their use here assumes that when
there
> is overflow, the output is not modified. That is what the inline assembly
does.
> But the intrinsics may/should modify the output, even when there is overflow:
> 
> https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html.
> 
> Ivan has suggested that I keep the changes here, and add TODOs to investigate
> using the intrinsics once we have something working.

Good catch! Alternative would be to just rewrite SMI_FAST_PATH_TOS to not pass
stack slot directly but only commit it if intrinsic succeeds.

[zra, 2016-04-25 15:42:56]

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (left):

https://codereview.chromium.org/1904153003/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:341: #if defined(__has_builtin)
On 2016/04/23 18:58:35, Vyacheslav Egorov (Google) wrote:
> On 2016/04/22 19:40:49, zra wrote:
> > On 2016/04/22 06:22:04, Vyacheslav Egorov (Google) wrote:
> > > Where did this go?
> > > 
> > > Inline assembly is just a fall back, compiler intrinsics are the right
thing
> > to
> > > rely on. 
> > > 
> > > I would expect that clang used to compile for ARM64 actually has support
for
> > > overflow intrinsics.
> > > 
> > 
> > I found the problem with the intrinsics. Their use here assumes that when
> there
> > is overflow, the output is not modified. That is what the inline assembly
> does.
> > But the intrinsics may/should modify the output, even when there is
overflow:
> > 
> > https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html.
> > 
> > Ivan has suggested that I keep the changes here, and add TODOs to
investigate
> > using the intrinsics once we have something working.
> 
> Good catch! Alternative would be to just rewrite SMI_FAST_PATH_TOS to not pass
> stack slot directly but only commit it if intrinsic succeeds. 

Assuming the intrinsic does a branch to decide the return value, we'd be adding
another branch to decide whether to commit. Maybe it doesn't matter because the
additional branch goes away after the compiler finishes inlining. We can
investigate that later. Right now, I'd like to land this CL to unblock wiring up
the interpreter for Flutter.

[Ivan Posva, 2016-04-25 19:39:57]

LGTM -ip

[zra, 2016-04-25 19:48:37]

Description was changed from

==========
DBC: Adds simdbc64 target, adds arm64 arithmetic overflow logic

There were differences between linux gcc, android gcc,
MacOS clang, and 32 vs. 64-bit w.r.t the __builtin 
functions for detecting arithmetic overflow. I couldn't get
them all working at the same time. Instead, I removed them,
and changed to always use the inline assembly. This works
in all the configurations above.

This change also adds a simdbc64 target for building simdbc
for 64-bit, and sets up the android targets. simdbc targets
arm, and simdbc64 targets arm64. You can build them with:

$ ./tools/build.py -m release -a simdbc{64} --os=android runtime
==========

to

==========
DBC: Adds simdbc64 target, adds arm64 arithmetic overflow logic

There were differences between linux gcc, android gcc,
MacOS clang, and 32 vs. 64-bit w.r.t the __builtin
functions for detecting arithmetic overflow. I couldn't get
them all working at the same time. Instead, I removed them,
and changed to always use the inline assembly. This works
in all the configurations above.

This change also adds a simdbc64 target for building simdbc
for 64-bit, and sets up the android targets. simdbc targets
arm, and simdbc64 targets arm64. You can build them with:

$ ./tools/build.py -m release -a simdbc{64} --os=android runtime

R=iposva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3d7202bad38c09b956a3f76bf87f736bd53ebb35
==========

[zra, 2016-04-25 19:48:38]

Committed patchset #6 (id:100001) manually as
3d7202bad38c09b956a3f76bf87f736bd53ebb35.