Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.

Update 64x2 instructions #115

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Update 64x2 instructions #115

merged 1 commit into from
Oct 1, 2019

Conversation

ngzhian
Copy link
Member

@ngzhian ngzhian commented Oct 1, 2019

A poll was held at CG meeting today, the consensus was for Option 3 of
#101.

This means we have all the f64x2 instructions, and some of the more
common i64x2 that have been benchmarked. This change reflects that
option in the proposal text, binary text, and implementation status.

Fixes #101.

@ngzhian ngzhian requested review from dtig and tlively October 1, 2019 17:52
@tlively
Copy link
Member

tlively commented Oct 1, 2019

Does the V8 implementation status need to be updated as well, or do we still have unimplemented 64x2 instructions?

@ngzhian
Copy link
Member Author

ngzhian commented Oct 1, 2019

Good point, they are only implemented for x64 and arm64 (and some for ia32), so I'm not sure if it counts as implemented or not.

Copy link
Member

@dtig dtig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the i64x2.trunc_sat_* operations be removed too? #101 lists them under Option 1.

@dtig
Copy link
Member

dtig commented Oct 1, 2019

For V8 implementation status, it's fully supported when the operations are implemented on all (officially) supported architectures to make sure that a binary that works on one platform does not fail decode with an unimplemented opcode on a different platform.

@ngzhian
Copy link
Member Author

ngzhian commented Oct 1, 2019

Shouldn't the i64x2.trunc_sat_* operations be removed too? #101 lists them under Option 1.

I was thinking of these as "conversion" operators, since these are i64x2 -> f64x2.

For V8 implementation status,

Thanks for clarifying, in that case the implementation status should be left empty.

Copy link
Member

@dtig dtig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for clarifying.

A poll was held at CG meeting today, the consensus was for Option 3 of
WebAssembly#101.

This means we have all the f64x2 instructions, and some of the more
common i64x2 that have been benchmarked. This change reflects that
option in the proposal text, binary text, and implementation status.

Fixes WebAssembly#101.
@ngzhian ngzhian merged commit 4e0a8e6 into WebAssembly:master Oct 1, 2019
| `i64x2.shl` | `0x87`| - |
| `i64x2.shr_s` | `0x88`| - |
| `i64x2.shr_u` | `0x89`| - |
| `i64x2.add` | `0x8a`| - |
| `i64x2.mul` | `0x8c`| - |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were reserving 0x90 for i64x2.mul.
i8x16.mul(0x5d) = i8x16.neg(0x51) + 12
i16x8.mul(0x6e) = i16x8.neg(0x62) + 12
i32x4.mul(0x7f) = i32x4.neg(0x73) + 12
i64x2.mul(0x8c) = i64x2.neg(0x84) + 8?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I don't think these are final yet, we will look at the actual opcodes again once we have a complete set. There will probably be reordering required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were pretty intentional in laying out the opcode space about a year ago before we started adding new instructions, so I think we should respect those decisions where possible.

Copy link
Member Author

@ngzhian ngzhian Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to change the opcode of i64x2.mul, but with new instructions like the load_splat and load_extend, the space of i8x16 and the space of i64x2 will not be the same size anymore, and this nice equation mapping *.neg to *.mul will conflict with the goal of keeping all the i8x16 instructions together (supposing there is such a goal).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I agree that for new instructions like load_splat, load_extend, narrowing, and widening just putting them at the end of the opcode space for now is the best move.

@ngzhian ngzhian deleted the 64x2 branch October 4, 2019 17:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-introduce some 64x2 instructions
4 participants