[turbofan] Add Int64(Add|Sub)WithOverflow support.

[turbofan] Add Int64(Add|Sub)WithOverflow support.

Committed: https://crrev.com/bafb568b6e9e70968d9b26f506b1cefe89a6b28a
Cr-Commit-Position: refs/heads/master@{#33039}

[Jarin, 2015-12-22 10:12:57]

jarin@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Jarin, 2015-12-22 10:16:24]

@v8-mips-ports, could you please help me with the mips64 port?

Benedikt, could you please take a look at the rest? (The new operator is not
used anywhere yet.)

[balazs.kilvady, 2015-12-22 10:19:30]

On 2015/12/22 10:16:24, Jarin wrote:
> @v8-mips-ports, could you please help me with the mips64 port?
We are on it.

[Benedikt Meurer, 2015-12-22 10:32:24]

LGTM with nits.

https://codereview.chromium.org/1544743004/diff/1/src/base/bits.h
File src/base/bits.h (right):

https://codereview.chromium.org/1544743004/diff/1/src/base/bits.h#newcode219
src/base/bits.h:219: uint64_t res = static_cast<uint64_t>(lhs) +
static_cast<uint64_t>(rhs);
Nit: Use __builtin_saddll_overflow here if available.

https://codereview.chromium.org/1544743004/diff/1/src/base/bits.h#newcode229
src/base/bits.h:229: uint64_t res = static_cast<uint64_t>(lhs) -
static_cast<uint64_t>(rhs);
Nit: Use __builtin_ssubll_overflow here if available.

[Jarin, 2015-12-22 10:42:21]

https://codereview.chromium.org/1544743004/diff/1/src/base/bits.h
File src/base/bits.h (right):

https://codereview.chromium.org/1544743004/diff/1/src/base/bits.h#newcode219
src/base/bits.h:219: uint64_t res = static_cast<uint64_t>(lhs) +
static_cast<uint64_t>(rhs);
On 2015/12/22 10:32:24, Benedikt Meurer wrote:
> Nit: Use __builtin_saddll_overflow here if available.

As discussed offline, I would like to avoid this because of the "long long"
versus "long" mess. (And also because I think it is better to have just code
path.)

[balazs.kilvady, 2015-12-22 18:55:20]

On 2015/12/22 10:16:24, Jarin wrote:
> @v8-mips-ports, could you please help me with the mips64 port?
> 

MIPS64 port is uploaded to https://codereview.chromium.org/1543883002/
Please take a look and merge it if it is acceptable.

[balazs.kilvady, 2015-12-23 14:18:10]

On 2015/12/22 18:55:20, balazs.kilvady wrote:
> On 2015/12/22 10:16:24, Jarin wrote:
> > @v8-mips-ports, could you please help me with the mips64 port?
> > 
> 
> MIPS64 port is uploaded to https://codereview.chromium.org/1543883002/
> Please take a look and merge it if it is acceptable.

Jarin,

On a mips64 target hardware only the
cctest/test-run-machops/RunInt64SubWithOverflowInBranchP fails (while it passes
on simulator). I investigated the results and with PrintF() I got this results:
$ out/mips64el.debug/cctest --random-seed=394360032 --turbo
test-run-machops/RunInt64SubWithOverflowInBranchP --nohard-abort
--nodead-code-elimination --nofold-constants
subOvf(0, 0) == int64: 0, int32: 0 -- bt.call(*i, *j) == int64: 0, int32: 0
subOvf(0, 1) == int64: -1, int32: -1 -- bt.call(*i, *j) == int64: -1, int32: -1
subOvf(0, 4294967295) == int64: -4294967295, int32: 1 -- bt.call(*i, *j) ==
int64: -4294967295, int32: 1


#
# Fatal error in ../test/cctest/compiler/test-run-machops.cc, line 599
# Check failed: expected == res (1 vs. -4294967295).
#

And the DCHECK() fails because of int32_t -> int64_t conversions (I guess on
simulator the conversion is compiled by clang to x86 CPU while on mips it was
compiled by gcc for MIPS64 CPU).

So could you apply one of the following diffs in your CL (beside the port:
https://codereview.chromium.org/1543883002/)?

diff --git a/test/cctest/compiler/test-run-machops.cc
b/test/cctest/compiler/test-run-machops.cc
index 1f1b661..ed3e944 100644
--- a/test/cctest/compiler/test-run-machops.cc
+++ b/test/cctest/compiler/test-run-machops.cc
@@ -593,7 +593,7 @@ TEST(RunInt64SubWithOverflowInBranchP) {
       if (!bits::SignedSubOverflow64(*i, *j, &result)) {
         expected = static_cast<int32_t>(result);
       }
-      CHECK_EQ(expected, bt.call(*i, *j));
+      CHECK_EQ(expected, static_cast<int32_t>(bt.call(*i, *j)));
     }
   }
 }


——————————————————
diff --git a/test/cctest/compiler/test-run-machops.cc
b/test/cctest/compiler/test-run-machops.cc
index 1f1b661..f1096c8 100644
--- a/test/cctest/compiler/test-run-machops.cc
+++ b/test/cctest/compiler/test-run-machops.cc
@@ -588,10 +588,10 @@ TEST(RunInt64SubWithOverflowInBranchP) {
   bt.AddReturn(val);
   FOR_INT64_INPUTS(i) {
     FOR_INT64_INPUTS(j) {
-      int32_t expected = constant;
+      int64_t expected = constant;
       int64_t result;
       if (!bits::SignedSubOverflow64(*i, *j, &result)) {
-        expected = static_cast<int32_t>(result);
+        expected = result;
       }
       CHECK_EQ(expected, bt.call(*i, *j));
     }

[Jarin, 2015-12-24 18:48:37]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2015-12-24 18:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1544743004/#ps60001
(title: "Add truncation to the int64-sub-with-overflow-branch test")

[commit-bot: I haz the power, 2015-12-24 18:48:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1544743004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1544743004/60001

[commit-bot: I haz the power, 2015-12-24 18:50:44]

Committed patchset #4 (id:60001)

[Jarin, 2015-12-24 18:50:46]

On 2015/12/23 14:18:10, balazs.kilvady wrote:
> On 2015/12/22 18:55:20, balazs.kilvady wrote:
> > On 2015/12/22 10:16:24, Jarin wrote:
> > > @v8-mips-ports, could you please help me with the mips64 port?
> > > 
> > 
> > MIPS64 port is uploaded to https://codereview.chromium.org/1543883002/
> > Please take a look and merge it if it is acceptable.

Thank you for the port! Merged now.

> Jarin,
> 
> On a mips64 target hardware only the
> cctest/test-run-machops/RunInt64SubWithOverflowInBranchP fails (while it
passes
> on simulator). I investigated the results and with PrintF() I got this
results:
> $ out/mips64el.debug/cctest --random-seed=394360032 --turbo
> test-run-machops/RunInt64SubWithOverflowInBranchP --nohard-abort
> --nodead-code-elimination --nofold-constants
> subOvf(0, 0) == int64: 0, int32: 0 -- bt.call(*i, *j) == int64: 0, int32: 0
> subOvf(0, 1) == int64: -1, int32: -1 -- bt.call(*i, *j) == int64: -1, int32:
-1
> subOvf(0, 4294967295) == int64: -4294967295, int32: 1 -- bt.call(*i, *j) ==
> int64: -4294967295, int32: 1

I think this is actually a bug in the test, good catch!

> 
> 
> #
> # Fatal error in ../test/cctest/compiler/test-run-machops.cc, line 599
> # Check failed: expected == res (1 vs. -4294967295).
> #
> 
> And the DCHECK() fails because of int32_t -> int64_t conversions (I guess on
> simulator the conversion is compiled by clang to x86 CPU while on mips it was
> compiled by gcc for MIPS64 CPU).
> 
> So could you apply one of the following diffs in your CL (beside the port:
> https://codereview.chromium.org/1543883002/)
> 
> diff --git a/test/cctest/compiler/test-run-machops.cc
> b/test/cctest/compiler/test-run-machops.cc
> index 1f1b661..ed3e944 100644
> --- a/test/cctest/compiler/test-run-machops.cc
> +++ b/test/cctest/compiler/test-run-machops.cc
> @@ -593,7 +593,7 @@ TEST(RunInt64SubWithOverflowInBranchP) {
>        if (!bits::SignedSubOverflow64(*i, *j, &result)) {
>          expected = static_cast<int32_t>(result);
>        }
> -      CHECK_EQ(expected, bt.call(*i, *j));
> +      CHECK_EQ(expected, static_cast<int32_t>(bt.call(*i, *j)));
>      }
>    }
>  }
> 
> 
> ——————————————————
> diff --git a/test/cctest/compiler/test-run-machops.cc
> b/test/cctest/compiler/test-run-machops.cc
> index 1f1b661..f1096c8 100644
> --- a/test/cctest/compiler/test-run-machops.cc
> +++ b/test/cctest/compiler/test-run-machops.cc
> @@ -588,10 +588,10 @@ TEST(RunInt64SubWithOverflowInBranchP) {
>    bt.AddReturn(val);
>    FOR_INT64_INPUTS(i) {
>      FOR_INT64_INPUTS(j) {
> -      int32_t expected = constant;
> +      int64_t expected = constant;
>        int64_t result;
>        if (!bits::SignedSubOverflow64(*i, *j, &result)) {
> -        expected = static_cast<int32_t>(result);
> +        expected = result;
>        }
>        CHECK_EQ(expected, bt.call(*i, *j));
>      }

[commit-bot: I haz the power, 2015-12-24 18:51:35]

Description was changed from

==========
[turbofan] Add Int64(Add|Sub)WithOverflow support.
==========

to

==========
[turbofan] Add Int64(Add|Sub)WithOverflow support.

Committed: https://crrev.com/bafb568b6e9e70968d9b26f506b1cefe89a6b28a
Cr-Commit-Position: refs/heads/master@{#33039}
==========

[commit-bot: I haz the power, 2015-12-24 18:51:36]

Patchset 4 (id:??) landed as
https://crrev.com/bafb568b6e9e70968d9b26f506b1cefe89a6b28a
Cr-Commit-Position: refs/heads/master@{#33039}