[wasm] add atomic opcodes

[wasm] add atomic opcodes

R=titzer@chromium.org,binji@chromium.org
BUG=v8:4614
Committed: https://crrev.com/4156d940e47122e322c073da41bd824f9b72be13
Cr-Commit-Position: refs/heads/master@{#40606}

[aseemgarg, 2016-10-17 21:06:02]

Description was changed from

==========
[wasm] add atomic opcodes

R=titzer@chromium.org,binji@chromium.org
BUG=
==========

to

==========
[wasm] add atomic opcodes

R=titzer@chromium.org,binji@chromium.org
BUG=v8:4614
==========

[aseemgarg, 2016-10-17 21:13:48]

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

[commit-bot: I haz the power, 2016-10-17 21:13:50]

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

[aseemgarg, 2016-10-17 21:52:57]

On 2016/10/17 21:05:50, aseemgarg wrote:

Still need to figure out if we should add load and store with the prefix or just
use the flag in existing load/store

[binji, 2016-10-17 21:59:26]

I think it would be best to have all atomic operations use a prefix, for
consistency.

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1112: if (opcode == kAtomicPrefix) {
seems like it might be nicer to roll this into the switch instead

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1294: void BuildAtomicOperator(WasmOpcode opcode) {}
This should error out

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/wasm-opcodes.cc
File src/wasm/wasm-opcodes.cc (right):

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/wasm-opcodes.c...
src/wasm/wasm-opcodes.cc:137:
kSimpleExprSigs[kAtomicExprSigTable[static_cast<byte>(opcode & 0xff)]]);
The mask is unnecessary (the others above don't do it)

[aseemgarg, 2016-10-17 22:08:48]

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1112: if (opcode == kAtomicPrefix) {
On 2016/10/17 21:59:26, binji wrote:
> seems like it might be nicer to roll this into the switch instead

Done.

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1294: void BuildAtomicOperator(WasmOpcode opcode) {}
On 2016/10/17 21:59:26, binji wrote:
> This should error out

Done.

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/wasm-opcodes.cc
File src/wasm/wasm-opcodes.cc (right):

https://codereview.chromium.org/2423883003/diff/20001/src/wasm/wasm-opcodes.c...
src/wasm/wasm-opcodes.cc:137:
kSimpleExprSigs[kAtomicExprSigTable[static_cast<byte>(opcode & 0xff)]]);
On 2016/10/17 21:59:26, binji wrote:
> The mask is unnecessary (the others above don't do it)

Unnecessary but seems like same convention is used for simd.

[titzer, 2016-10-21 07:49:41]

https://codereview.chromium.org/2423883003/diff/40001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2423883003/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1110: if (module_ && module_->origin == kAsmJsOrigin) {
Maybe we want a flag for wasm_atomics_prototype in flag-definitions.h?

https://codereview.chromium.org/2423883003/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1112: byte atomic_index = *(pc_ + 1);
You shouldn't deref *pc_ directly (I also just noticed the above code does
this). It should instead use a checked_read method.

[aseemgarg, 2016-10-24 23:16:18]

https://codereview.chromium.org/2423883003/diff/40001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2423883003/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1110: if (module_ && module_->origin == kAsmJsOrigin) {
On 2016/10/21 07:49:41, titzer wrote:
> Maybe we want a flag for wasm_atomics_prototype in flag-definitions.h?

Done.

https://codereview.chromium.org/2423883003/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1112: byte atomic_index = *(pc_ + 1);
On 2016/10/21 07:49:40, titzer wrote:
> You shouldn't deref *pc_ directly (I also just noticed the above code does
> this). It should instead use a checked_read method.

Done. Took care of others in a different patch.

[titzer, 2016-10-26 08:07:17]

lgtm with nits

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1154: if (!FLAG_wasm_atomics_prototype) {
You can use the macro for this check.

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1158: len++;
Please don't do len++, but set len = 2.

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1159: byte atomic_index = checked_read_u8(pc_, 1,
"atomic index");
"atomic opcode" instead of "atomic index"

[aseemgarg, 2016-10-26 19:53:57]

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1154: if (!FLAG_wasm_atomics_prototype) {
On 2016/10/26 08:07:17, titzer wrote:
> You can use the macro for this check.

The CHECK_PROTOTYPE_OPCODE checks that module origin is not asmjs. In this case
we want the origin to be asmjs. 
I can create a new Macro for that but since it is used at only one place yet,
don't really see the point.

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1158: len++;
On 2016/10/26 08:07:17, titzer wrote:
> Please don't do len++, but set len = 2.

Done. Can you explain why?

https://codereview.chromium.org/2423883003/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1159: byte atomic_index = checked_read_u8(pc_, 1,
"atomic index");
On 2016/10/26 08:07:17, titzer wrote:
> "atomic opcode" instead of "atomic index"

Done.

[aseemgarg, 2016-10-26 19:54:04]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2016-10-26 19:54:04]

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/2423883003/#ps80001
(title: "[wasm] add atomic opcodes")

[commit-bot: I haz the power, 2016-10-26 19:54:13]

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, 2016-10-26 20:28:10]

Description was changed from

==========
[wasm] add atomic opcodes

R=titzer@chromium.org,binji@chromium.org
BUG=v8:4614
==========

to

==========
[wasm] add atomic opcodes

R=titzer@chromium.org,binji@chromium.org
BUG=v8:4614
==========

[commit-bot: I haz the power, 2016-10-26 20:28:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-17 22:14:49]

Description was changed from

==========
[wasm] add atomic opcodes

R=titzer@chromium.org,binji@chromium.org
BUG=v8:4614
==========

to

==========
[wasm] add atomic opcodes

R=titzer@chromium.org,binji@chromium.org
BUG=v8:4614

Committed: https://crrev.com/4156d940e47122e322c073da41bd824f9b72be13
Cr-Commit-Position: refs/heads/master@{#40606}
==========

[commit-bot: I haz the power, 2016-11-17 22:14:50]

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