Add wasm internal opcodes for asm.js stdlib functions we're missing.

Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org,yangguo@chromium.org
LOG=N

Committed: https://crrev.com/4db99810da9e2cc02dda10faa2be7ae896318ba3
Cr-Commit-Position: refs/heads/master@{#34452}

[titzer, 2016-02-24 17:54:11]

https://codereview.chromium.org/1729833002/diff/20001/src/wasm/asm-wasm-build...
File src/wasm/asm-wasm-builder.cc (right):

https://codereview.chromium.org/1729833002/diff/20001/src/wasm/asm-wasm-build...
src/wasm/asm-wasm-builder.cc:986: case AsmTyper::kMathAbs: {
We're going to need some more extensive tests for these stdlib functions. E.g.
we don't transform Math.abs calls into Float64Abs in TurboFan because of (I
think) a semantic difference. It's ok for integers but we should test reasonable
inputs for the floating point types.

https://codereview.chromium.org/1729833002/diff/20001/src/wasm/asm-wasm-build...
src/wasm/asm-wasm-builder.cc:992: byte code[] = {WASM_I32(0)};
WASM_I8

[bradn, 2016-02-24 18:07:28]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2016-02-24 18:07:29]

https://codereview.chromium.org/1729833002/diff/20001/src/wasm/asm-wasm-build...
File src/wasm/asm-wasm-builder.cc (right):

https://codereview.chromium.org/1729833002/diff/20001/src/wasm/asm-wasm-build...
src/wasm/asm-wasm-builder.cc:986: case AsmTyper::kMathAbs: {
On 2016/02/24 17:54:11, titzer wrote:
> We're going to need some more extensive tests for these stdlib functions. E.g.
> we don't transform Math.abs calls into Float64Abs in TurboFan because of (I
> think) a semantic difference. It's ok for integers but we should test
reasonable
> inputs for the floating point types.

Yeah this one is goofy, Math.abs is supposed to handle signed -> signed, but
Math.abs(-(1<<32)) seems to yield a value that's really an intish.

Agreed on the tests. I plan to go through these with a fine tooth comb for
boundary cases, but am focusing on bringing up embenchen for the moment.

https://codereview.chromium.org/1729833002/diff/20001/src/wasm/asm-wasm-build...
src/wasm/asm-wasm-builder.cc:992: byte code[] = {WASM_I32(0)};
On 2016/02/24 17:54:11, titzer wrote:
> WASM_I8

Done.

https://codereview.chromium.org/1729833002/diff/20001/src/wasm/asm-wasm-build...
src/wasm/asm-wasm-builder.cc:1009: current_function_builder_->Emit(kExprIfElse);
Min and max have the same -(1<<31) issue it seems too, I'll ask Alon about it
later.

[aseemgarg, 2016-02-25 22:38:37]

This seems fine. But I am not too sure of the design choice. If we are going the
external function route, can we at least use names that are not legal asm names
to ensure no conflicts. 
Also (we can leave that as a TODO for now), we should probably handle adding
these other functions to ffi automatically in instantiate module rather than
explicitly in JS code.

[bradnelson, 2016-03-02 03:23:18]

Description was changed from

==========
For now, route stdlib functions that aren't opcodes to ffi.

Call an ffi function for stdlib functions used in an asm.js
module (for those that don't have opcodes).

This will need to change to run correctly with unmodified asm.js

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org
LOG=N
==========

to

==========
Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org
LOG=N
==========

[bradnelson, 2016-03-02 03:49:12]

https://codereview.chromium.org/1729833002/diff/80001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1729833002/diff/80001/src/assembler.cc#newcod...
src/assembler.cc:1281: if (std::isinf(x) && std::isinf(y)) {
This is lifted from Runtime_atan2

Should I move this at a shared function in assembler.h/cc the way modulo is ?

[titzer, 2016-03-02 19:16:55]

On 2016/03/02 03:49:12, bradnelson wrote:
> https://codereview.chromium.org/1729833002/diff/80001/src/assembler.cc
> File src/assembler.cc (right):
> 
>
https://codereview.chromium.org/1729833002/diff/80001/src/assembler.cc#newcod...
> src/assembler.cc:1281: if (std::isinf(x) && std::isinf(y)) {
> This is lifted from Runtime_atan2
> 
> Should I move this at a shared function in assembler.h/cc the way modulo is ?

As discussed offline, it's fine to leave it with a TODO.

lgtm

[bradn, 2016-03-02 19:17:16]

The CQ bit was checked by bradnelson@google.com

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

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

[commit-bot: I haz the power, 2016-03-02 19:20:49]

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

[commit-bot: I haz the power, 2016-03-02 19:20:50]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11798)

[bradn, 2016-03-02 19:28:59]

Description was changed from

==========
Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org
LOG=N
==========

to

==========
Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org,yangguo@chromium.org
LOG=N
==========

[bradn, 2016-03-02 19:29:09]

bradnelson@google.com changed reviewers:
+ yangguo@chromium.org

[bradn, 2016-03-02 19:29:32]

+yangguo for OWNERS in src/snapshot

[Yang, 2016-03-02 19:39:46]

On 2016/03/02 19:29:32, bradn wrote:
> +yangguo for OWNERS in src/snapshot

lgtm

[yangguo, 2016-03-02 19:41:44]

On 2016/03/02 19:39:46, Yang wrote:
> On 2016/03/02 19:29:32, bradn wrote:
> > +yangguo for OWNERS in src/snapshot
> 
> lgtm

I'm curious though. Do you expect any wasm code to end up in either the startup
snapshot of the code cache? If not, all of these additional external references
is just going to bloat the external reference table and add additional overhead
to create it.

[yangguo, 2016-03-02 19:42:11]

On 2016/03/02 19:41:44, yangguo wrote:
> On 2016/03/02 19:39:46, Yang wrote:
> > On 2016/03/02 19:29:32, bradn wrote:
> > > +yangguo for OWNERS in src/snapshot
> > 
> > lgtm
> 
> I'm curious though. Do you expect any wasm code to end up in either the
startup
> snapshot of the code cache? If not, all of these additional external
references
> is just going to bloat the external reference table and add additional
overhead
> to create it.

...startup snapshot *or* the code cache...

[titzer, 2016-03-02 20:12:39]

On 2016/03/02 19:41:44, yangguo wrote:
> On 2016/03/02 19:39:46, Yang wrote:
> > On 2016/03/02 19:29:32, bradn wrote:
> > > +yangguo for OWNERS in src/snapshot
> > 
> > lgtm
> 
> I'm curious though. Do you expect any wasm code to end up in either the
startup
> snapshot of the code cache? If not, all of these additional external
references
> is just going to bloat the external reference table and add additional
overhead
> to create it.

Yes, we will want to eventually cache WASM compiled code for an entire module.
Probably don't want to put it in the startup snapshot :-)

[aseemgarg, 2016-03-03 01:17:04]

lgtm

I see a lot of code repetition. Should we use macros for some of these?

[bradn, 2016-03-03 01:19:20]

Perhaps. Although as these have snapshot footprint, explicit might be good.

[bradn, 2016-03-03 01:19:28]

The CQ bit was checked by bradnelson@google.com

[commit-bot: I haz the power, 2016-03-03 01:19:32]

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

[commit-bot: I haz the power, 2016-03-03 01:22:09]

Description was changed from

==========
Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org,yangguo@chromium.org
LOG=N
==========

to

==========
Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org,yangguo@chromium.org
LOG=N
==========

[commit-bot: I haz the power, 2016-03-03 01:22:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-03 01:23:29]

Description was changed from

==========
Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org,yangguo@chromium.org
LOG=N
==========

to

==========
Add wasm internal opcodes for asm.js stdlib functions we're missing.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org,yangguo@chromium.org
LOG=N

Committed: https://crrev.com/4db99810da9e2cc02dda10faa2be7ae896318ba3
Cr-Commit-Position: refs/heads/master@{#34452}
==========

[commit-bot: I haz the power, 2016-03-03 01:23:30]

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