Skip to content

Implement v128.{load,store}{8,16,32,64}_lane instructions #3278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 23, 2020

Conversation

tlively
Copy link
Member

@tlively tlively commented Oct 22, 2020

These instructions are proposed in WebAssembly/simd#350.
This PR implements them throughout Binaryen except in the C/JS APIs and in the
fuzzer, where it leaves TODOs instead. Right now these instructions are just
being implemented for prototyping so adding them to the APIs isn't critical and
they aren't generally available to be fuzzed in Wasm engines.

These instructions are proposed in WebAssembly/simd#350.
This PR implements them throughout Binaryen except in the C/JS APIs and in the
fuzzer, where it leaves TODOs instead. Right now these instructions are just
being implemented for prototyping so adding them to the APIs isn't critical and
they aren't generally available to be fuzzed in Wasm engines.
@tlively tlively requested review from kripken and aheejin October 22, 2020 19:33
ret->align = align;
ret->index = index;
ret->ptr = ptr;
ret->finalize();
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this does not assign vec?

(I wonder if an automatic code generation tool could help in the future here... 🤔)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! Yes, it certainly would.

@@ -1302,6 +1302,9 @@ static size_t parseMemAttributes(Element& s,
align = fallbackAlign;
while (!s[i]->isList()) {
const char* str = s[i]->c_str();
if (strncmp(str, "align", 5) != 0 && strncmp(str, "offset", 6) != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, what does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

These instructions are the first to have a memarg (align and offset) followed by another immediate argument. This parsing routine previously expected the align and offset to be the only non-expression operands (see the loop condition), so it wasn't able to parse the new instructions correctly. This addition makes the loop bail out instead of raise an error when it encounters a non-memarg immediate argument.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks. Maybe a comment on the function (saying what it parses, and that it bails on anything else)?

@@ -1302,6 +1302,9 @@ static size_t parseMemAttributes(Element& s,
align = fallbackAlign;
while (!s[i]->isList()) {
const char* str = s[i]->c_str();
if (strncmp(str, "align", 5) != 0 && strncmp(str, "offset", 6) != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks. Maybe a comment on the function (saying what it parses, and that it bails on anything else)?

@tlively tlively merged commit daf0782 into WebAssembly:master Oct 23, 2020
@tlively tlively deleted the simd-load-store-lane branch October 23, 2020 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants