[wasm] Fix interpreter entry for i64 return type

[wasm] Fix interpreter entry for i64 return type

Fix two issues in the interpreter entry for 64 bit return values on
32 bit platforms. First, the effect chain was slightly incorrect, second
the order of the returned values was wrong.

Also add a test case for this.
Tested on x64, ia32 and s390.

Plus drive-by fix in Int64Lowering to reuse global constants for
big-endian/little-endian disambiguation.

R=titzer@chromium.org
BUG=v8:5822
Review-Url: https://codereview.chromium.org/2731713002
Cr-Commit-Position: refs/heads/master@{#43654}
Committed: https://chromium.googlesource.com/v8/v8/+/6cf8f54d900c0f0cb940bfd272539b6876df35bd

[Clemens Hammacher, 2017-03-03 13:16:45]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 13:16:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 13:44:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 13:44:30]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-03-03 13:53:05]

Description was changed from

==========
[wasm] Fix interpreter entry for i64 return type

Fix two issues in the interpreter entry for 64 bit return values on
32 bit platforms. First, the effect chain was slightly incorrect, second
the order of the returned values was wrong.

Also add a test case for this.
Tested on x64, ia32 and s390.

R=titzer@chromium.org
CC=ivica.bogosavljevic@imgtec.com, bjaideep@ca.ibm.com
BUG=v8:5822
==========

to

==========
[wasm] Fix interpreter entry for i64 return type

Fix two issues in the interpreter entry for 64 bit return values on
32 bit platforms. First, the effect chain was slightly incorrect, second
the order of the returned values was wrong.

Also add a test case for this.
Tested on x64, ia32 and s390.

R=titzer@chromium.org
BUG=v8:5822
==========

[Clemens Hammacher, 2017-03-07 10:07:06]

Ping

or should I add ahaas@?

[titzer, 2017-03-07 10:21:55]

lgtm with nits

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2976: auto is_i64_as_two_params =
[&](wasm::ValueType t) {
Can you extract this to a utility function in Int64Lowering?

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:3033: Int32Constant(4), lower, *control_);
Can you keep the sizeof(int32_t) for documentation here?

[Clemens Hammacher, 2017-03-07 10:30:03]

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2976: auto is_i64_as_two_params =
[&](wasm::ValueType t) {
On 2017/03/07 at 10:21:55, titzer wrote:
> Can you extract this to a utility function in Int64Lowering?

Int64Lowering current does the lowering unconditionally. Adding the
machine()->Is32() check there would not fit the rest of this class.

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:3033: Int32Constant(4), lower, *control_);
On 2017/03/07 at 10:21:55, titzer wrote:
> Can you keep the sizeof(int32_t) for documentation here?

Done.

[titzer, 2017-03-07 11:51:52]

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2976: auto is_i64_as_two_params =
[&](wasm::ValueType t) {
On 2017/03/07 10:30:03, Clemens Hammacher wrote:
> On 2017/03/07 at 10:21:55, titzer wrote:
> > Can you extract this to a utility function in Int64Lowering?
> 
> Int64Lowering current does the lowering unconditionally. Adding the
> machine()->Is32() check there would not fit the rest of this class.

What I meant is just a static function that takes a MachineOperatorBuilder and a
ValueType and returns true/false.

It might be useful elsewhere and it's probably best to put it in Int64Lowering.

[Clemens Hammacher, 2017-03-07 12:10:37]

Description was changed from

==========
[wasm] Fix interpreter entry for i64 return type

Fix two issues in the interpreter entry for 64 bit return values on
32 bit platforms. First, the effect chain was slightly incorrect, second
the order of the returned values was wrong.

Also add a test case for this.
Tested on x64, ia32 and s390.

R=titzer@chromium.org
BUG=v8:5822
==========

to

==========
[wasm] Fix interpreter entry for i64 return type

Fix two issues in the interpreter entry for 64 bit return values on
32 bit platforms. First, the effect chain was slightly incorrect, second
the order of the returned values was wrong.

Also add a test case for this.
Tested on x64, ia32 and s390.

Plus drive-by fix in Int64Lowering to reuse global constants for
big-endian/little-endian disambiguation.

R=titzer@chromium.org
BUG=v8:5822
==========

[Clemens Hammacher, 2017-03-07 12:19:49]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 12:19:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Clemens Hammacher, 2017-03-07 12:21:33]

On 2017/03/07 at 11:51:52, titzer wrote:
>
https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler.cc
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2731713002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:2976: auto is_i64_as_two_params =
[&](wasm::ValueType t) {
> On 2017/03/07 10:30:03, Clemens Hammacher wrote:
> > On 2017/03/07 at 10:21:55, titzer wrote:
> > > Can you extract this to a utility function in Int64Lowering?
> > 
> > Int64Lowering current does the lowering unconditionally. Adding the
> > machine()->Is32() check there would not fit the rest of this class.
> 
> What I meant is just a static function that takes a MachineOperatorBuilder and
a ValueType and returns true/false.
> 
> It might be useful elsewhere and it's probably best to put it in
Int64Lowering.

Ah, I see. Done.

As a drive-by fix, I also removed redundant constants from Int64Lowering
(kLowerWordOffset and kHigherWordOffset) and replaced with existing
kInt64LowerHalfMemoryOffset and kInt64UpperHalfMemoryOffset.

[commit-bot: I haz the power, 2017-03-07 12:45:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 12:45:23]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-03-07 15:12:19]

lgtm

[Clemens Hammacher, 2017-03-07 18:00:08]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:00:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 18:03:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488909608426030,
"parent_rev": "269983f388151dfa1726ec4e075e06e9d9737643", "commit_rev":
"6cf8f54d900c0f0cb940bfd272539b6876df35bd"}

[commit-bot: I haz the power, 2017-03-07 18:03:13]

Description was changed from

==========
[wasm] Fix interpreter entry for i64 return type

Fix two issues in the interpreter entry for 64 bit return values on
32 bit platforms. First, the effect chain was slightly incorrect, second
the order of the returned values was wrong.

Also add a test case for this.
Tested on x64, ia32 and s390.

Plus drive-by fix in Int64Lowering to reuse global constants for
big-endian/little-endian disambiguation.

R=titzer@chromium.org
BUG=v8:5822
==========

to

==========
[wasm] Fix interpreter entry for i64 return type

Fix two issues in the interpreter entry for 64 bit return values on
32 bit platforms. First, the effect chain was slightly incorrect, second
the order of the returned values was wrong.

Also add a test case for this.
Tested on x64, ia32 and s390.

Plus drive-by fix in Int64Lowering to reuse global constants for
big-endian/little-endian disambiguation.

R=titzer@chromium.org
BUG=v8:5822

Review-Url: https://codereview.chromium.org/2731713002
Cr-Commit-Position: refs/heads/master@{#43654}
Committed:
https://chromium.googlesource.com/v8/v8/+/6cf8f54d900c0f0cb940bfd272539b6876d...
==========

[commit-bot: I haz the power, 2017-03-07 18:03:17]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/6cf8f54d900c0f0cb940bfd272539b6876d...