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

Fix codegen for non-power-of-2 simd reprs. #72

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

pythonesque
Copy link
Contributor

The fix to change Vec3 and other non-power-of-2 simd vectors into repr_c temporarily didn't quite end up covering everything. Specifically, the actual codegen for the repr_simd types was left untouched, so when you tried to compile something that was previously using repr_simd::Vec3, it would fail at link time. Moreover, just changing the affected ones to repr_c in client code while leaving the others as repr_simd would entail a lot of code changes, since conversions between stuff like repr_c::Vec3 and repr_simd::Vec4 don't exist. This patch fixes things so that repr_simd::Vec3 and others provides an identical interface to what it did before.

@yoanlcq
Copy link
Owner

yoanlcq commented Mar 3, 2021

Hi, thanks for taking the time to make this!

Right now I'm afraid I don't understand the issue you were facing and how it could occur.

Back when I wrote the "fix", the only thing I did was to change the #[repr] attribute for non-power-of-two vectors in the repr_simd module; doing so does not change the existing API in any way, it just changes the struct's alignment constraint and what the compiler can assume about it. Clients are still dealing with the same types and APIs, at least that's supposed to be the case.

when you tried to compile something that was previously using repr_simd::Vec3, it would fail at link time

How so? Could you please show an example of what caused it? I might see it happening if one were passing a repr_simd::Vec3 to another API that expected a "true" SIMD vector, could that be something like it?

This patch fixes things so that repr_simd::Vec3 and others provides an identical interface to what it did before.

This confuses me; the interface never changed in the first place, only the #[repr] attribute did. I don't consider it as part of the interface, and I don't think it's a good idea for anyone to do so.

Looking several times at the diff, I was not able to wrap my head around the fix you suggest. I mean, perhaps it is the completely correct thing to do! But I'd be grateful if you could explain a bit more what it does exactly, or at least give some kind of overview.

Thanks!

@pythonesque
Copy link
Contributor Author

when you tried to compile something that was previously using repr_simd::Vec3, it would fail at link time

How so? Could you please show an example of what caused it? I might see it happening if one were passing a repr_simd::Vec3 to another API that expected a "true" SIMD vector, could that be something like it?

I already fixed it so I don't really want to have to hunt down the code can broke (but @zesterer and others can confirm it was an issue), but here's the explanation: all of the SIMD operations are resolved at link time. Specifically, wherever we branch using choose!($repr_c_or_simd, ...) we have two branches: the simd and C branch. The SIMD branch calls simd operations sometimes which don't work on non-SIMD vectors, so after your changes the code fails to link if you actually try to use the repr_simd::Vec3 with stuff like add.

What my fix does is additionally pass by a "repr_c_or_simd_non_powers_of_two" which we use for the non power of 2 vectors instead of repr_c_or_simd, so we can make sure to generate the corret repr_c config. However, since in one case (conversion functions) repr_c_or_simd was used to actually control the implementation of some functions (e.g. into_repr_c), I also had to pass a second thing around to handle that case. Overall this results in the same APIs as before, just with the C implementations used instead of the SIMD ones even when repr_simd is selected, for non powers of 2.

@yoanlcq
Copy link
Owner

yoanlcq commented Mar 3, 2021

Thank you! I get it now. I had completely forgotten about choose!.

Off the top of my head, I can't help but wonder if there was a slightly simpler way that would reduce the amount of arguments added to macros. Something like : only add a single macro parameter that will be designed as "the 1st argument for choose!" and that's it.

But your fix still looks close enough to ideal, so let's merge this!

(Also, I should probably put in place some process that could detect link time errors like the ones you encountered...)

@yoanlcq yoanlcq merged commit c04f361 into yoanlcq:master Mar 3, 2021
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.

2 participants