[wasm] WasmRunner can run tests with I64 parameters and return value.

[wasm] WasmRunner can run tests with I64 parameters and return value.

I extended the Int64Lowering to lower calls, loads, stores, returns, and
parameters and apply the lowering on both the test function TF graph and
the WasmRunner TF graph.

The lowering of calls also requires an adjustment of the call descriptor.

R=titzer@chromium.org

Committed: https://crrev.com/545943db155acf1594dfe1a57e455f656fc15192
Cr-Commit-Position: refs/heads/master@{#34121}

[ahaas, 2016-02-17 18:29:04]

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

[commit-bot: I haz the power, 2016-02-17 18:29:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704033002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704033002/1

[commit-bot: I haz the power, 2016-02-17 19:03:08]

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

[commit-bot: I haz the power, 2016-02-17 19:03:09]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-02-18 08:21:32]

https://codereview.chromium.org/1704033002/diff/1/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1704033002/diff/1/src/compiler/linkage.h#newc...
src/compiler/linkage.h:312: explicit Linkage(const CallDescriptor* incoming) :
incoming_(incoming) {}
CallDescriptors are immutable, so no need to add this C++ goop everywhere.

[ahaas, 2016-02-18 10:07:12]

On 2016/02/18 at 08:21:32, titzer wrote:
> https://codereview.chromium.org/1704033002/diff/1/src/compiler/linkage.h
> File src/compiler/linkage.h (right):
> 
>
https://codereview.chromium.org/1704033002/diff/1/src/compiler/linkage.h#newc...
> src/compiler/linkage.h:312: explicit Linkage(const CallDescriptor* incoming) :
incoming_(incoming) {}
> CallDescriptors are immutable, so no need to add this C++ goop everywhere.

I changed const CallDescriptor to CallDescriptor now.

[ahaas, 2016-02-18 10:59:39]

On 2016/02/18 at 10:07:12, ahaas wrote:
> On 2016/02/18 at 08:21:32, titzer wrote:
> > https://codereview.chromium.org/1704033002/diff/1/src/compiler/linkage.h
> > File src/compiler/linkage.h (right):
> > 
> >
https://codereview.chromium.org/1704033002/diff/1/src/compiler/linkage.h#newc...
> > src/compiler/linkage.h:312: explicit Linkage(const CallDescriptor* incoming)
: incoming_(incoming) {}
> > CallDescriptors are immutable, so no need to add this C++ goop everywhere.
> 
> I changed const CallDescriptor to CallDescriptor now.

I was able to remove all const CallDescriptor changes now.

[titzer, 2016-02-18 12:06:23]

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/int64-lowe...
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/int64-lowe...
src/compiler/int64-lowering.cc:35: #if WASM_64
Why not just return?

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1912: void
WasmGraphBuilder::Int64LoweringForTesting(
Why don't you just use the function_signature from this graph builder?

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2134: #if !WASM_64
You should use if (kPointerSize == 4) here; the WASM_64 macro was intended to
mean "64-bit ints are supported in WASM"

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm.cc:2496: BUILD(r,
WASM_I32_CONVERT_I64(WASM_CALL_FUNCTION(
You should use some real int64 values here.

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/wasm-r...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:354: #if !WASM_64
I don't understand what this signature is for. Seems bogus.

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:436: desc =
module->GetI32WasmCallDescriptor(this->zone(), desc);
Same here. You need to use kPointerSize == 4

[ahaas, 2016-02-18 13:00:18]

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/int64-lowe...
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/int64-lowe...
src/compiler/int64-lowering.cc:35: #if WASM_64
On 2016/02/18 at 12:06:22, titzer wrote:
> Why not just return?

I replaced the UNREACHABLE by a DCHECK_EQ(4, kPointerSize). I want to see it if
I ever use the lowering accidentally.

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1912: void
WasmGraphBuilder::Int64LoweringForTesting(
On 2016/02/18 at 12:06:22, titzer wrote:
> Why don't you just use the function_signature from this graph builder?

Done.

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1914: #if !WASM_64
I also replaced the #if !WASM_64 here.

https://codereview.chromium.org/1704033002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2134: #if !WASM_64
On 2016/02/18 at 12:06:22, titzer wrote:
> You should use if (kPointerSize == 4) here; the WASM_64 macro was intended to
mean "64-bit ints are supported in WASM"

Done.

I changed that also in Int64LoweringForTesting.

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm.cc:2496: BUILD(r,
WASM_I32_CONVERT_I64(WASM_CALL_FUNCTION(
On 2016/02/18 at 12:06:22, titzer wrote:
> You should use some real int64 values here.

Done.

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/wasm-r...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:354: #if !WASM_64
On 2016/02/18 at 12:06:22, titzer wrote:
> I don't understand what this signature is for. Seems bogus.

I need the signature of the wrapper as Signature<MachineRepresentation> to
initialize the Int64Lowering. signature_, which stores more or less the same
data, has type Signature<MachineType>.

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:356: Signature<MachineRepresentation>::Builder
rep_builder(zone(), 1, 4);
I introduced a new #define for the maximum number of parameters of the
WasmRunner to replace the "4" here and at other places.

https://codereview.chromium.org/1704033002/diff/40001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:436: desc =
module->GetI32WasmCallDescriptor(this->zone(), desc);
On 2016/02/18 at 12:06:23, titzer wrote:
> Same here. You need to use kPointerSize == 4

Done.

[titzer, 2016-02-18 13:38:31]

LGTM other than one assert

https://codereview.chromium.org/1704033002/diff/60001/src/compiler/int64-lowe...
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/1704033002/diff/60001/src/compiler/int64-lowe...
src/compiler/int64-lowering.cc:35: DCHECK_EQ(4, kPointerSize);
Please don't assert that. We'll want to write unittests that run on all
platforms.

[ahaas, 2016-02-18 13:53:25]

On 2016/02/18 at 13:38:31, titzer wrote:
> LGTM other than one assert
> 
>
https://codereview.chromium.org/1704033002/diff/60001/src/compiler/int64-lowe...
> File src/compiler/int64-lowering.cc (right):
> 
>
https://codereview.chromium.org/1704033002/diff/60001/src/compiler/int64-lowe...
> src/compiler/int64-lowering.cc:35: DCHECK_EQ(4, kPointerSize);
> Please don't assert that. We'll want to write unittests that run on all
platforms.

Done.

[ahaas, 2016-02-18 15:01:31]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-02-18 15:01:31]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1704033002/#ps80001
(title: "remove DCHECK")

[commit-bot: I haz the power, 2016-02-18 15:01:48]

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

[commit-bot: I haz the power, 2016-02-18 15:18:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-18 15:19:23]

Description was changed from

==========
[wasm] WasmRunner can run tests with I64 parameters and return value.

I extended the Int64Lowering to lower calls, loads, stores, returns, and
parameters and apply the lowering on both the test function TF graph and
the WasmRunner TF graph.

The lowering of calls also requires an adjustment of the call descriptor.

R=titzer@chromium.org
==========

to

==========
[wasm] WasmRunner can run tests with I64 parameters and return value.

I extended the Int64Lowering to lower calls, loads, stores, returns, and
parameters and apply the lowering on both the test function TF graph and
the WasmRunner TF graph.

The lowering of calls also requires an adjustment of the call descriptor.

R=titzer@chromium.org

Committed: https://crrev.com/545943db155acf1594dfe1a57e455f656fc15192
Cr-Commit-Position: refs/heads/master@{#34121}
==========

[commit-bot: I haz the power, 2016-02-18 15:19:24]

Patchset 5 (id:??) landed as
https://crrev.com/545943db155acf1594dfe1a57e455f656fc15192
Cr-Commit-Position: refs/heads/master@{#34121}