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

Used inline SIMD vectors if they are constants #2122

Merged
merged 7 commits into from
Jun 14, 2022

Conversation

gfoidl
Copy link
Contributor

@gfoidl gfoidl commented May 18, 2022

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Starting with .NET 5 constant SIMD vectors can be used / created "inline", so that the JIT can read the values from the data section. Thus the need of static readonly fields for these kind of vectors is gone.

Codegen (x64) was spot-checked. Basically it's

-      mov       rax,19D98002F38
-      mov       rax,[rax]
-      vmovupd   xmm8,[rax+8]
+      vmovupd   xmm0,[7FFF734BC5F0]

For loops or other code that isn't tiered compiled by the JIT, the static init check needed by the runtime is also elided with this change.

In micro-benchmarks that change allone I expect to be just below noise, as all data is in the cache and the branch-predictor (static init check) will be trainined. The difference could (hopefully be should) be visible in real-world apps.

Note: for micro-benchmarks #2121 will have an effect, see machine code comparison over there.

Fixes #1762
Contributes to #2121

Patterns applied

  • If a constant vector is used only once outside a loop, then Vector{128,256}.Create is called in-place of the argument. That way the JIT is able to produce code like

    vmulps reg, reg, [mem]

    and no additional (logical) register is needed.

  • If a constant vector is used once inside a loop, then the Vector{128,256}.Create is hoisted above the loop(s), as the JIT (currently) won't do this for us.

  • If a constant vector is used several times outside a loop, then Vector{128,256}.Create is assigned to a vector variable as early as possible (to hide the latency of loading from mem), but not too far away to keep the logical context of the code.

#endif

static Vp8Encoding()
private static byte[] GetClip1()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the static constructor (cctor) this method is now called from the static readonly field.
Avoiding a cctor and using beforefieldinit (IL) is the prefered way in dotnet/runtime too, as there are several pro reasons for it, liking timing, static init check, etc.

That's the only other code change not mentioned in PR's description.

@CLAassistant
Copy link

CLAassistant commented May 18, 2022

CLA assistant check
All committers have signed the CLA.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented May 19, 2022

@gfoidl

Yep, see #2117

Not sure what the cause is yet. It's only happening with the latest .NET 7 preview 4.

If you update your branch from main those tests will be skipped for now on that platform.

@gfoidl
Copy link
Contributor Author

gfoidl commented May 19, 2022

The CI leg for windows-latest is failing with

...ImageDifferenceIsOverThresholdException : Image difference is over threshold!
Test Environment OS : Windows
Test Environment is CI : True
Test Environment is .NET Core : True
Report ImageFrame 0:
Total difference: 29.9761%
...

Local tests are passing (VS, standard checkout as given in ReadMe).
Please give me some advice on how to investigate this.

@gfoidl
Copy link
Contributor Author

gfoidl commented May 19, 2022

@JimBobSquarePants thanks, now we had a race condition.
The short comment (now deleted) GH created while copying the error message (don't know why).

Thanks!

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing. Been focusing on one of the other libraries.

This all looks good to me. I think I'll still follow up with a PR to introduce a complete set of constants for MMShuffle though.

@JimBobSquarePants
Copy link
Member

I'm merging this. It's the same Skew test failing each time as noted in #2117

@JimBobSquarePants JimBobSquarePants merged commit 75dc96a into SixLabors:main Jun 14, 2022
@gfoidl gfoidl deleted the inline-vector-constants branch June 14, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise how constant SIMD vectors are defined
4 participants