Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

lane mutators #110

Closed
johnmccutchan opened this issue Jan 22, 2015 · 19 comments
Closed

lane mutators #110

johnmccutchan opened this issue Jan 22, 2015 · 19 comments

Comments

@johnmccutchan
Copy link
Collaborator

Instead of withX, withY how about something like replaceLane(int lane, T value)?

@johnmccutchan
Copy link
Collaborator Author

@PeterJensen @sunfishcode

@sunfishcode
Copy link
Member

That covers the non-constant index case of LLVM insertelement, which is nice to have.

On the other hand, what platforms support a non-constant index without using memory operations?

@PeterJensen
Copy link
Collaborator

A slightly better name would be insertLane(), since the instruction is insertps.

My main concern here is that the insertps/extractps instructions require that lane indicators are constants. We impose this limitation implicitly with the with* operations. With this suggestion we'll need to check that 'lane' is a constant or alternatively allow non-constant 'lane' values, which would complicate the code generation. Another concern is the duality of lane indicators, using both x/y/z/w and integers. I much prefer the letters, because use of integers tends to cause big/little endian portability issues.

@johnmccutchan
Copy link
Collaborator Author

Using lane indexes will make us consistent with shuffle / swizzle methods which already use indices.

It will be easy for runtimes to fast path constant lane index and fall back to a slow path if the index isn't constant.

@sunfishcode
Copy link
Member

All systems I'm aware of, including big-endian, store whatever they call SIMD lane 0 at offset 0 in memory, so I think we're safe from endianness issues here.

I agree that insertLane is a little nicer name than replaceLane.

Emscripten already has code to lower variable-index insertelement/extractelement into memory operations, which is pretty close to what JITs will lower variable-index replaceLane/insertLane into on all major platforms today anyway AFAIK, and it doesn't come up all that often in use cases I'm looking at, so I don't feel strongly either way about this feature.

Speaking of extractelement, if we add replaceLane/insertLane, should we add a variable-index extractLane too, for symmetry?

@PeterJensen
Copy link
Collaborator

There was a good talk at the LLVM dev meeting about big/little endian SIMD issues. It seems that Power looks at it the other way around: http://llvm.org/devmtg/2014-10/Slides/Schmidt-SupportingVectorProgramming.pdf

@sunfishcode
Copy link
Member

Oh, I was unaware of that. On big-endian Power, LLVM already presents little-endian element ordering (what the presentation might call Little-on-Big) to its users, but I didn't realize that's not what the actual hardware does.

The presentation observes that "The True-LE model is appropriate and useful to most consumers", and I believe that that's what makes the most sense for SIMD.js too.

@sunfishcode
Copy link
Member

Given that we already now use indices for shuffle/swizzle, and if we use indices here, the only things left using "xyzw" notation are the element getters and {load,store}{X,XY,XYZ}. I think it makes sense to make those consistent too. And I encourage the names "insert" and "extract", following LLVM's use of those names. So, I propose we:

  • replace the lane mutators with SIMD.<type>.insertLane(index, value)
  • replace the lane getters with SIMD.<type>.extractLane(index)
  • rename {load,store}{X,XY,XYZ} to {load,store}{1,2,3}
  • explicitly require that SIMD loads and stores transfer vector elements to SIMD lanes with little-endian ordering for the lane indices so that the element at memory offset 0 is always at SIMD index 0

This eliminates the last "xyzw" naming from the API, which is a little sad for programs where that naming fits the domain, but we gain consistency across SIMD types, we gain the ability to represent LLVM's insertelement and extractelement with non-constant indices, and it's still pretty usable, given that this is a low-level API.

@johnmccutchan
Copy link
Collaborator Author

sgtm

@PeterJensen
Copy link
Collaborator

Yes! I think we're converging on a very nice and consistent API. One nitpick though; I like .setLane()/.getLane() better. They're shorter names and arguably express the operation better. I think it was John that said use of 'insert' sort of implies an addition not a replacement.

@huningxin
Copy link
Contributor

sgtm! Thanks for the proposal @sunfishcode.

@huningxin
Copy link
Contributor

rename {load,store}{X,XY,XYZ} to {load,store}{1,2,3}

load2 might confuse people that it means loading lane 2, does it? How about {load,store}{1,2,3}Lane(s)?

@sunfishcode , your thoughts?

@sunfishcode
Copy link
Member

Our current convention is for operations that only act on one lane to have Lane in the name, so a load that only loads lane 2 (if we were to add such a thing) would be SIMD.type.loadLane2(a, i) or SIMD.type.loadLane(2, a, i) or something.

load2 for loading 2 elements feels right to me, and load2Lanes feels unnecessarily verbose. It is just a feeling though.

@johnmccutchan
Copy link
Collaborator Author

I agree with @sunfishcode load2 sounds better to me than load2Lanes.

@huningxin
Copy link
Contributor

Thanks for your comments. It makes sense to me. I put together a PR to rename the load/store API #136. Please take a look.

@bnjbvr
Copy link
Contributor

bnjbvr commented Apr 16, 2015

Nice! It seems that #136 only addressed the third bullet point of @sunfishcode 's list, should we re-open the issue so as to not forget about renaming other functions?

@huningxin
Copy link
Contributor

Agree with @bnjbvr . #136 only updates the load/store APIs. Before we close it, we need consensus on .setLane()/.getLane() or .insertLane()/.extractLane().

@PeterJensen
Copy link
Collaborator

We did reach consensus at the 4/14 meeting. The consensus was .replaceLane()/.extractLane(). @sunfishcode felt that use of the get/set verbs might create unwanted associations to getter and setter functions.

@huningxin
Copy link
Contributor

Thanks for the clarification. I will come up with a PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants