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

Small rename of shuffle and swizzle #316

Closed
ngzhian opened this issue Aug 18, 2020 · 10 comments · Fixed by #321
Closed

Small rename of shuffle and swizzle #316

ngzhian opened this issue Aug 18, 2020 · 10 comments · Fixed by #321

Comments

@ngzhian
Copy link
Member

ngzhian commented Aug 18, 2020

I propose to rename v8x16.shuffle and v8x16.swizzle to i8x16.shuffle and i8x16.swizzle, this is a tiny change from v prefix, to i.

The purpose of the v is to indicate that we are not concerned with the underlying type. The rename does not change this fact. The purpose of the rename is to make the spec slightly simpler, and have less special cases.

If #297 happens (likely, given lack of opposition), these 2 instructions will be the only special instructions that begin with v8x16. If we can use i8x16.swizzle and i8x16.shuffle, it will simplify the specification slightly, since there is no need to talk about v8x16 as a syntax.

Additionally, the rename will be consistent with other instructions, like the horizontal reductions i32x4.all_true, which applies the same if the instruction was f32x4.all_true.

@binji
Copy link
Member

binji commented Aug 24, 2020

You could bring this to the SIMD group, but I'm guessing there won't be much opposition, since changing the name doesn't really affect much. Personally I think we should go ahead and make these changes now.

ngzhian added a commit to ngzhian/simd that referenced this issue Aug 24, 2020
v8x16.shuffle and v8x16.swizzle is now i8x16.shuffle and i8x16.swizzle
respectively.

Fixed WebAssembly#316.
@penzn
Copy link
Contributor

penzn commented Aug 25, 2020

I think this only makes sense in context of the other change you are proposing, otherwise it would not be the last remaining instruction with v8x16 name. Can we bring this up as part of the large rename, please?

@ngzhian
Copy link
Member Author

ngzhian commented Aug 25, 2020

@penzn "bring this up as port of the large rename" do you mean refer to this issue in the issue with the larger rename? Or bring both of these topics out at the next SIMD sync?

@penzn
Copy link
Contributor

penzn commented Aug 25, 2020

I think we should adopt the larger rename of loads and this one at the same time. I also think we should do another sync, though I doubt there would be much opposition to these changes.

@ngzhian
Copy link
Member Author

ngzhian commented Aug 25, 2020

Got it, in comment#1:

If #297 happens (likely, given lack of opposition), these 2 instructions will be the only special instructions that begin with v8x16. If we can use i8x16.swizzle and i8x16.shuffle, it will simplify the specification slightly, since there is no need to talk about v8x16 as a syntax.

So if that large rename happens, then this will also happen alongside. (If the large rename happens there is even less reason this issue should be dropped.)

@penzn
Copy link
Contributor

penzn commented Aug 25, 2020

With larger rename happening there is no reason to keep the v8x16 on shuffle and swizzle, at least in my opinion :)

@ngzhian
Copy link
Member Author

ngzhian commented Aug 25, 2020

With larger rename happening there is no reason to keep the v8x16 on shuffle and swizzle, at least in my opinion :)

I am in agreement! :)

ngzhian added a commit to ngzhian/simd that referenced this issue Sep 4, 2020
v8x16.shuffle and v8x16.swizzle is now i8x16.shuffle and i8x16.swizzle
respectively.

Fixed WebAssembly#316.
@carlsmith
Copy link

carlsmith commented Sep 6, 2020

The shuffle instruction doesn't really shuffle anything. To shuffle implies randomly mixing the items in one array, not cherrypicking items from two. I would suggest pick or choose or something like that.

@tlively
Copy link
Member

tlively commented Sep 6, 2020

@carlsmith I agree that it's not totally self documenting, but "shuffle" is standard terminology in SIMD ISAs and other SIMD-related work. For example, see clang's portable __builtin_shufflevector function. For users already familiar with SIMD, this is the clearest name.

@carlsmith
Copy link

@tlively - Ah, I see. That makes sense then. Sorry. Thanks for the clarification.

ngzhian added a commit that referenced this issue Sep 8, 2020
v8x16.shuffle and v8x16.swizzle is now i8x16.shuffle and i8x16.swizzle
respectively.

Fixed #316.
tlively added a commit to llvm/llvm-project that referenced this issue Dec 22, 2020
These instructions previously used prefixes like v8x16 to signify that they were
agnostic between float and int interpretations. We renamed these instructions to
remove this form of prefix in WebAssembly/simd#297 and
WebAssembly/simd#316 and this commit brings the names
in LLVM up to date.

Differential Revision: https://reviews.llvm.org/D93722
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 a pull request may close this issue.

5 participants