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

Conversation

@ngzhian
Copy link
Member

@ngzhian ngzhian commented Sep 10, 2020

@ngzhian ngzhian force-pushed the execution-semantics branch 2 times, most recently from 29d8f35 to 1651eb9 Compare September 12, 2020 00:05
@ngzhian
Copy link
Member Author

ngzhian commented Sep 12, 2020

@binji I'm still working on shuffle, swizzle, load splat and load extends, the rest of the ops are mostly done, definitely still needs cleanup. Hoping you can take a preliminary look.

Copy link
Member Author

@ngzhian ngzhian left a comment

Choose a reason for hiding this comment

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

I left some review notes (numbered based on suggested ordering to look at) to highlight bigger changes or extensions to existing definition.

@ngzhian ngzhian force-pushed the execution-semantics branch from 221a375 to 9e786cc Compare September 14, 2020 23:24
@binji
Copy link
Member

binji commented Sep 14, 2020

Did a pass on it, I really like extending the numerics, makes it much nicer to read IMO

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Some preliminary comments, haven't read all of exec/instructions yet.

@ngzhian ngzhian marked this pull request as ready for review September 16, 2020 20:34
@ngzhian ngzhian force-pushed the execution-semantics branch 2 times, most recently from 1a94e8c to b6b4497 Compare September 22, 2020 20:46
@ngzhian
Copy link
Member Author

ngzhian commented Dec 3, 2020

@rossberg Thanks for the careful review. I have resolved all the minor issues. Left a couple of responses and questions for you. And apologies for not doing the recent changes at one go. I missed that there was a "Show X hidden conversations" button that had a couple of your review notes, and so I didn't fix those issues during my first go.

ngzhian and others added 2 commits December 3, 2020 01:25
Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
@ngzhian
Copy link
Member Author

ngzhian commented Dec 3, 2020

As a checklist, the remaining open comments are (listing here since some of the comments are hidden):

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Great! Thanks for bearing with me.

@rossberg
Copy link
Member

rossberg commented Dec 4, 2020

Oops, I just noticed that there's a related issue with vtrunc and vconvert, whose execution does not map to that of a regular op either, because it involves two shapes. I think you'll need to introduce a vcvtop category and handle that separately. (Also, they are currently included in vbinop, which does not even seem correct for typing).

Feel free to fix that in a separate PR, if you prefer.

@ngzhian
Copy link
Member Author

ngzhian commented Dec 4, 2020

I think you'll need to introduce a vcvtop category and handle that separately.

Good catch, thanks. Added a new production, vcvtop, which has its own validation and execution.

The way I'm writing them now relies on this new note on execution/numerics: https://github.com/WebAssembly/simd/pull/340/files#diff-14d634364829defe70ba02a1ed0075266fce32b208b681eb5a812e96104e2bf9R41
My intention is that this also applies to the conversion operators, is that a correct assumption? Or should I clarify that section with an example showing how conversion operators work when given a list of inputs?

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM modulo comment.

Yes, the note should suffice. If you want to emphasise that it applies to heterogeneously typed ops like conversions as well, you could replace the simple fabs example that follows with convert.

@ngzhian ngzhian merged commit 890c043 into WebAssembly:master Dec 8, 2020
@ngzhian ngzhian deleted the execution-semantics branch December 8, 2020 02:20
ngzhian added a commit to ngzhian/simd that referenced this pull request Dec 8, 2020
@ngzhian ngzhian added the spectext changes to spec text label Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

spectext changes to spec text

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants