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

Does not compile on latest nightly (SIMD vector's only field must be an array) #96

Open
martin-t opened this issue Sep 21, 2024 · 5 comments

Comments

@martin-t
Copy link
Contributor

The feature repr_simd no longer supports structs with multiple fields.

The fix is to convert vek's vectors to use arrays internally. I haven't looked at how much work it would be.

The workaround is to disable vek's default features and only use the ones you need - e.g. vek = { version = "0.17.0", default-features = false, features = ["rgba", "rgb", "std"] }.

I propose that a quick temporary solution would be to publish 0.17.1 with repr_simd removed from the list of default features.

@yoanlcq
Copy link
Owner

yoanlcq commented Sep 22, 2024

Thanks for reporting this.

I could probably write an essay about how much I hate this change, but what really matters are the actions I take.

My knee jerk reaction is : at this point the most sensible thing to do is for vek to drop repr_simd support. What's the point anymore? std::simd::f32x4 and friends seem to be the way forward.
So you'd use vek's repr_c vectors for general gameplay code, and as soon as you need really performance-critical batch calculations, you go all in with arrays of f32x4 and clever specialized code.
To be fair, this has always been the way to go in C/C++.

And that's it. The alternative ("solution 2") is to still support repr_simd but replace members by a single array only for these, and state clearly that they can't act as a drop-in replacement for repr_c types anymore. People would still be able to convert between these two repr if they like; that, or they replace their uses of the x, y, z, w members.

Your proposal to remove repr_simd from default features is a good idea. It's the least I can do.

But man... This is going to be a pain. Granted, we're in unstable territory, but it's still something that's suddenly coming up out of nowhere to eat my time, and there's nothing in it for me. Just a free waste of time. Gotta love it

@martin-t
Copy link
Contributor Author

martin-t commented Oct 1, 2024

Thanks for the quick update.

I wonder if there's a way to fight this change. It seems this whole change, both the proposal and implementation, was done by one person. And although the justification is about simplifying the code and the chance affects many files, most of them are tests and the change to real code is small. Finally, they never even considered that people could be using named fields, all they talked about was tuple structs.

Correct me if i am wrong but this feature was the only way to have sane notation like my_vec.x while also allowing the vector to benefit from SIMD. After this change, if you want SIMD, you would need to wrote my_vec[0], right?

If so, this is a major usability downgrade. One of rust's massive flaws that hit me repeatedly in gamedev is its lack of fields in traits and here we run into it again because the best interface you can achieve now is my_vec.x().

What do you think about writing a post on https://internals.rust-lang.org/ to argue for undoing this change?

@yoanlcq
Copy link
Owner

yoanlcq commented Oct 5, 2024

There are unfortunately a lot of counterarguments, and because I know them, I can't go out there and stand my ground in good faith:

  • The change was approved more than a year ago;
  • We are the only ones so far who have expressed concern over it, which I feel is way too few;
  • In all fairness, I understand their point of view; having to support 3 (members + tuples + arrays) different ways of doing anything is always a lot more annoying than having just 1; it leaves room for opinionated designs and fragmentation, from compiler internals to the ecosystem itself;
  • The vec[0] and vec.x() are annoying for sure, but they will be 100% future-proof; no matter what change they make later, nothing will prevent us from having this interface.
  • In C, SIMD vector types have no members, element access is done via array notation (to have member access syntax, you have to use union). In general, it seems that the "state of the art" everywhere is array notation and indexing.
  • In most libs/engines I've seen (including an in-house engine in a professional setting), vector types which allow the .x syntax are not SIMD, and that's never where performance issues lie (e.g Unreal Engine's FVector is 3 doubles; everybody is forced to accept it. Their SIMD vector type is VectorRegister and has to be indexed like an array). This sounds like copium but leads me to my next point :
  • Actual, efficient use of SIMD is actually more than just making Vec4 use it. It's more about using well-known instructions for well-known register sizes for well-known hardware, and completely changing your data layout to make best use of it; anything less is, as far as I dislike this fact, micro-optimization. What I'm getting at, essentially, is that repr_simd is not a magic switch; it may do some good of course, but the gains will pale in comparison to specialized code.

But perhaps there's another way 🤔 I just wrote a small experiment with Deref, and I feel it might just work. Playground link.

It remains to be seen whether that could still make "going from repr_c to repr_simd" non-seamless, but we're already past that point.
I'll be a bit busy due to real life stuff but I'll try to work on it soon.

@yoanlcq
Copy link
Owner

yoanlcq commented Oct 5, 2024

My workaround wouldn't allow for construction by members or destructuration :

let v = simd::Vec4 { x: ... ... };
let simd::Vec4 { x, y, ... } = v;

the most "direct" equivalent would be to use From/Into :

let v: simd::Vec4 = c::Vec4 { x: ... ... }.into();
let c::Vec4 { x, y, ... } = v.into();

A slight inconvenience... Could be worse though

@martin-t
Copy link
Contributor Author

martin-t commented Oct 5, 2024

  • The change was approved more than a year ago;

  • We are the only ones so far who have expressed concern over it, which I feel is way too few;

The proposal, yes, but the implementation was only merged now and i found the issue as soon as it was merged because i use nightly on CI. I agree it's weird nobody else has noticed though.

  • The vec[0] and vec.x() are annoying for sure, but they will be 100% future-proof

This is death by a thousand papercuts for gamedev. Having so much extra special characters as noise in a complex expression is painful and i really wish Rust lang designers understood that.

  • Actual, efficient use of SIMD is actually more than just making Vec4 use it.

That's a fair point, i've never benchmarked my game because TBH perf is rarely an issue in (rust) gamedev. Getting things done is :)

I just wrote a small experiment with Deref

Good idea, that could work. I also "abuse" Deref and transmute to access fields on different structs as if they had the same interface and i had 0 issues so far.

My workaround wouldn't allow for construction by members or destructuration

Papercuts. Though in these particular cases, I don't mind and it's a good tradeoff given the choices.

I'll be a bit busy due to real life stuff but I'll try to work on it soon.

Understandable, I don't have time to work on my game now either, i only noticed because I set CI to run weekly even with no commits. And everything runs perfectly OK without SIMD as well.

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

No branches or pull requests

2 participants