Skip to content
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

Add arch functions for all non-Matrix arithmetic instructions #446

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

XAMPPRocky
Copy link
Member

This PR adds every arithmetic instruction in SPIR-V, except anything related to Matrixes, as I wanted to make a separate PR for that.

There's two functions for every instruction (e.g. i_add/i_add_vector), as I couldn't really find a way to make a function generic over a vector or a scalar, so I just split the implementation into two functions.

Depends on #441 for the const generic in vectors.

@XAMPPRocky XAMPPRocky requested review from eddyb and khyperia February 23, 2021 12:23
@XAMPPRocky XAMPPRocky force-pushed the arith-vec-ops branch 2 times, most recently from 38e58e3 to 69e753d Compare February 23, 2021 12:53
@khyperia
Copy link
Contributor

khyperia commented Feb 23, 2021

Do we have a need for the scalar functions here? They're already exposed via the traditional operators, feels a little weird to create intrinsics for things we already support in a very built-in way. (I imagined the arch module is for intrinsics that are specific to SPIR-V and cannot be exposed via traditional language constructs, in the same way that core::arch::x86 doesn't expose an intrinsic for "add two integers together")

Also want to loop in @Jasper-Bekkers as a loong while back they were against vector ops, I think we've discussed it more recently and might have changed their mind, but I want to make sure.

Also, how do you imagine the vector ops being integrated into glam? I don't want to merge this without a path forward of how to use these for the operator overloads. (Kind of skipped the design question of what we're going to use these for and how)

@XAMPPRocky
Copy link
Member Author

Do we have a need for the scalar functions here? They're already exposed via the traditional operators

There are some scalar functions here that don't map to an operator. Such as modulo, as % implemented as remainder in Rust. I added the rest for completeness and consistencies' sake, but I don't feel too strongly about their inclusion.

Also, how do you imagine the vector ops being integrated into glam? I don't want to merge this without a path forward of how to use these for the operator overloads. (Kind of skipped the design question of what we're going to use these for and how)

That's a good question, and I think it will require either tightening the coupling with glam so that's not an extern crate (essentially having it as a submodule in the repo that is linked in as a module in the source), this would allow us to implement operator traits for glam types in rust-gpu.

The other option would be decoupling glam from spirv-std, so that glam can depend on spirv-std and then glam can add and maintain the trait impls themselves. This would require removing Sealed requirement for these types, but we can always hide the traits from the docs, and there isn't really an issue if someone else does makes a vector crate that is compatible with rust-gpu, in fact that might be a benefit.

Either option would work I think, tradeoff's are more about who's maintaining the implementation of the link between glam <-> rust-gpu.

@khyperia
Copy link
Contributor

There are some scalar functions here that don't map to an operator. Such as modulo, as % implemented as remainder in Rust. I added the rest for completeness and consistencies' sake, but I don't feel too strongly about their inclusion.

Modulo is exposed as rem_euclid, no?

@XAMPPRocky
Copy link
Member Author

Modulo is exposed as rem_euclid, no?

Huh, I had forgot about that one, good point. Though seeing it now, I do remember some bike shedding over that name. Regardless, I can't see to be able to use that? I tried it on playground and locally and I can't seem to be able to use rem_euclid.

http://shader-playground.timjones.io/ec14ba2de19f76c7460e88d7c04bf09a

@Jasper-Bekkers
Copy link
Contributor

Also want to loop in @Jasper-Bekkers as a loong while back they were against vector ops, I think we've discussed it more recently and might have changed their mind, but I want to make sure.

One of the trade-offs I had missed at the time was the significantly smaller SPIR-V binaries having vector ops would generate. I think this is a good inclusion. 👍

@khyperia
Copy link
Contributor

khyperia commented Mar 1, 2021

Huh, I had forgot about that one, good point. Though seeing it now, I do remember some bike shedding over that name. Regardless, I can't see to be able to use that? I tried it on playground and locally and I can't seem to be able to use rem_euclid.

Looks like num-traits doesn't have it, we should probably push it through so rust-gpu "just works". rust-num/num-traits#159

@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented Mar 2, 2021

I've nudged the PR on num-traits along so that should come eventually. @khyperia So what should I do with this PR? Remove all the non-vector instructions?

@khyperia
Copy link
Contributor

khyperia commented Mar 2, 2021

@khyperia So what should I do with this PR? Remove all the non-vector instructions?

I think so! If you have a reasonably clear plan of how to implement vector ops for glam op overloads with this, then yeah!

@XAMPPRocky XAMPPRocky force-pushed the arith-vec-ops branch 2 times, most recently from cf28432 to d8b48a9 Compare March 11, 2021 13:03
@XAMPPRocky XAMPPRocky force-pushed the arith-vec-ops branch 3 times, most recently from e45a69d to 0c2c055 Compare March 12, 2021 09:50
@XAMPPRocky XAMPPRocky requested review from fu5ha and VZout as code owners March 12, 2021 09:50
@XAMPPRocky
Copy link
Member Author

This should be good to go, but needs #476 to land first.

@XAMPPRocky XAMPPRocky enabled auto-merge (squash) March 16, 2021 09:18
@XAMPPRocky XAMPPRocky merged commit 1d36549 into main Mar 16, 2021
@XAMPPRocky XAMPPRocky deleted the arith-vec-ops branch March 16, 2021 09:42
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.

3 participants