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

Revise how constant SIMD vectors are defined #1762

Closed
gfoidl opened this issue Sep 14, 2021 · 6 comments · Fixed by #2122
Closed

Revise how constant SIMD vectors are defined #1762

gfoidl opened this issue Sep 14, 2021 · 6 comments · Fixed by #2122
Milestone

Comments

@gfoidl
Copy link
Contributor

gfoidl commented Sep 14, 2021

Starting with .NET 5 the JIT emits code to read constant SIMD vectors from the data section which gives nice machine code and some related advantages like the ones listed in #1761 (comment).
See also dotnet/runtime#44115 for more context on that issue.

But changing the usage of constant vectors to the "inline variant" will regress pre .NET 5 targets, thus it seems best to keep current code with static readonlys, and update to the inline variant once .NET Core 3.1 will go out of support on 3rd Dec 2022 (the purpose of this issue is to have a remainder for that task).

@antonfirsov
Copy link
Member

antonfirsov commented Sep 14, 2021

3rd Dec 2022 is pretty far, ideally we will release ImageSharp 3.0 before that, and personally I wouldn't remove .NET Core 3.1 support without bumping ImageSharp's major version, so I'm afraid it's not anything for the near future.

Would an inclined property be equivalent of "inline vector create"? Can we do a trick like following?

#if NET5_OR_NEWER
private static Vector256<float> Constant =>  Vector256.Create(0.707106781f);
#else
private static readonly  Vector256<float> Constant = Vector256.Create(0.707106781f);
#endif

I would only do it if there is measurable benefit justifying the increased complexity.

@gfoidl
Copy link
Contributor Author

gfoidl commented Sep 14, 2021

The trick seems to work (at least for simple cases, where it's easy for the JIT to inline the property).

But, similar as you wrote, this makes the code awful to read. So I wouldn't do this now and keep the code that works for .NET Core 3.1 and .NET 5 onwards.
Once .NET 3.1 support will be dropped, this issue should be addressed in a batch-change. Code remains clean now and then.

wouldn't remove .NET Core 3.1 support without bumping ImageSharp's major version, so I'm afraid it's not anything for the near future.

Agree.

@br3aker
Copy link
Contributor

br3aker commented Sep 14, 2021

I would only do it if there is measurable benefit justifying the increased complexity.

There's no benefit, at least in microbenchmarking: #1761 (comment).
While this does indeed produce better code it won't really change anything globally so I guess waiting for the end of core 3.1 is the way to go.

@antonfirsov antonfirsov added this to the Future milestone Sep 14, 2021
@JimBobSquarePants JimBobSquarePants modified the milestones: Future, 3.0.0 Apr 20, 2022
@JimBobSquarePants
Copy link
Member

We're .NET 6 now so if someone wants to update the code to reflect the new pattern you have my blessing.

@gfoidl
Copy link
Contributor Author

gfoidl commented May 13, 2022

I can look into this next week.

@JimBobSquarePants
Copy link
Member

Awesome @gfoidl thanks!

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 a pull request may close this issue.

4 participants