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

Tracking issue for glam refactoring #304

Closed
Jasper-Bekkers opened this issue Dec 2, 2020 · 7 comments
Closed

Tracking issue for glam refactoring #304

Jasper-Bekkers opened this issue Dec 2, 2020 · 7 comments
Labels
t: enhancement A new feature or improvement to an existing one.

Comments

@Jasper-Bekkers
Copy link
Contributor

Our main vector math crate, glam-rs is currently being refactored to remove code duplication. This is an issue for tracking that progress and to make sure glam-rs continues to work with these changes.

Glam issue: bitshifter/glam-rs#99
Glam branch: https://github.com/bitshifter/glam-rs/tree/refactor

@Jasper-Bekkers Jasper-Bekkers added the t: enhancement A new feature or improvement to an existing one. label Dec 2, 2020
@bitshifter
Copy link

I have rebased the spriv changes from master into my refactor branch and changed attributes so that outer types like Vec3 are repr(transparent) and the inner types that I'm using like XYZ<T> are repr(simd) for the spirv target. It seems to compile and run default example in rust-gpu. I assume though that someone will need to check the codegen to be sure that this is working correctly? I don't know how to do that.

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Dec 4, 2020

I assume though that someone will need to check the codegen to be sure that this is working correctly? I don't know how to do that.

So you know, once you've built for spirv-unknown-unknown you should have a spv file in your target you can use with spirv-dis tool that comes with SPIR-V tools to look at a pretty printed version of the assembly.

@bitshifter
Copy link

bitshifter commented Dec 6, 2020

I've made a gist with the diassasmbled spirv comparing glam master to glam refactor here: https://gist.github.com/bitshifter/dcf83dc148b05a58d4fb1753949aa4fe

I think the repr(transparent) outer type with the repr(simd) inner type is working, but perhaps someone with familiarity with SPIR-V could check.

It looks like the use of traits is bloating the spirv and preventing inlining in some cases. It would be good to know if this is a Rust GPU compiler issue or a more general problem. I haven't got to the point where I was looking at performance and codegen on CPU, but I guess now I am worried that my current approach might have too much of a performance penalty. Anyway, it would be good to better understand the implications on the Rust GPU side. Thanks. I'm not locked in to my current solution although if if performance and codegen seems OK on regular Rust I would most likely stick with it, unless it turns out that it's going to cause a lot of problems for Rust GPU.

@bitshifter
Copy link

This is at the point where the bulk of the work has been done, f64, i32 and u32 types have been added. Performance for f32 seems as good or better than before (for the SSE2 implementation). It might be good if someone from Embark could check that this branch isn't going to cause any bit issues for rust-gpu. I still have a bunch of tidying to do on the branch, but I imagine it will make it into master sometime in the next couple of weeks.

@bitshifter
Copy link

This has been merged to master. It's a pretty significant refactor so any branches are most likely going to have conflicts. LMK if you have any questions.

@khyperia
Copy link
Contributor

Sorry we've been quiet on this! Just smoke-tested and tried building rust-gpu against glam master, everything seems fine.

I don't seem to be reproducing the OpName bloating shenanigans, so idk, but in general we're probably not too worried about binary size and performance right now, we're more focused on just making things work.

@khyperia
Copy link
Contributor

khyperia commented Apr 1, 2021

Refactor completed! Probably should have closed this a while ago.

@khyperia khyperia closed this as completed Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants