Suboptimal code patterns when using Unsafe methods + intrinsics

[redknightlois]

Micro-optimizing a particular piece of code I found suboptimal codegen introduced by the signature of the Unsafe class design that could be fixed by the JIT.

Say that I need to read data from 2 different memory locations where offset is an int

matches = Sse2.MoveMask(Sse2.CompareEqual(LoadVector128(ref first, (IntPtr)offset), LoadVector128(ref second, (IntPtr)offset)));

Now you can see that it is performing 2 times the same operation.

**movsxd      r8,eax  
vmovupd     xmm0,xmmword ptr [rcx+r8]  
**movsxd      r8,eax  
vmovupd     xmm1,xmmword ptr [rdx+r8]  
vpcmpeqb    xmm0,xmm0,xmm1  
vpmovmskb   r8d,xmm0  

This has been solved (somehow) for AVX2 but it also introduce another strange behavior:

matches = Avx2.MoveMask(Avx2.CompareEqual(LoadVector256(ref first, (IntPtr)offset), LoadVector256(ref second, (IntPtr)offset)));

As you can see not only we copy with sign extension but we are also coping it into r9. While at the architectural level that is a simple rename (better than the other one) we are still issuing an extra operation.

**movsxd      r8,eax  
**mov         r9,r8  
vmovupd     ymm0,ymmword ptr [rcx+r9]  
vmovupd     ymm1,ymmword ptr [rdx+r8]  
vpcmpeqb    ymm0,ymm0,ymm1  
vpmovmskb   r8d,ymm0  

What I dont understand is why if eax has been set in the same code (not coming from anywhere else) the JIT decides to use an extra mov operation instead of emitting:

vmovupd     ymm0,ymmword ptr [rcx+eax]  
vmovupd     ymm1,ymmword ptr [rdx+eax]  
vpcmpeqb    ymm0,ymm0,ymm1  
vpmovmskb   r8d,ymm0  

And, futhermore, this can also be optimized to:

vmovupd     ymm0,ymmword ptr [rcx+eax]  
vpcmpeqb    ymm0,ymm0,ymmword ptr [rdx+eax]  
vpmovmskb   r8d,ymm0  

I am running nightly from today. 3.0.0-preview4-27506-5
Any idea how I can achieve the latter code?

category:cq
theme:hardware-intrinsics
skill-level:expert
cost:medium

[tannergooding]

Is this a scenario where you aren't or can't pin the underlying data?_

[redknightlois]

I can write this code to work entirely pointer based, but I was tasked into looking into potential improvements for SequenceCompareTo as it is core to our workload and we are looking to not having to maintain our own memory compare routines :)

Ben's version is using this method of accessing the data, so I presume there is a reason why he is not pinning it.

[redknightlois]

Well apparently converting the int into long would get rid of most of the issues.

So the issue is caused by the JIT not registering over the codepath that it can promote the int value into long early and forget about it afterwards. Then that will ripple over to the whole code generating multiple suboptimal code sequences all over the place.

[tannergooding]

Ben's version is using this method of accessing the data, so I presume there is a reason why he is not pinning it.

I wonder if exposing LoadVector128(ref T) overloads (in addition to the LoadVector128(T*) overloads) would help here. We had originally only exposed the pointer overloads because:

1. It was thought that most code using intrinsics would be operating on larger data
2. It was thought that most code using intrinsics would therefore pin the data, to ensure cache-locality, alignment, etc
3. It was thought that ref T might be confusing since it could load just T or it could load more.

I've seen the above but also seen a number of cases where users are using intrinsics for smaller sets of inputs or where they aren't pinning the data (for various reasons).

CC. @jkotas, @CarolEidt. Should we reconsider whether both ref T and T* overloads are exposed for the handful of Load/Store intrinsics that exist or should we continue just exposing T* overloads and have users workaround this by using Unsafe to get a vector from a ref T where required?

[redknightlois]

The other issue appears to be related to https://github.com/dotnet/coreclr/issues/19521. I have code to test it when it is fixed :) cc @CarolEidt

[jkotas]

@jkotas, @CarolEidt. Should we reconsider whether both ref T and T*

I do not think we should be encouraging the ref pointer arithmetic. The ref pointer arithmetic is prone to subtle intermittent bugs that are very hard to trace down. I understand that we are leaving a bit of perf on the table in some cases by omitting support for the ref pointer arithmetic, but I am ok with that.

[redknightlois]

@jkotas I haven't seen any huge codegen issue when using ref. Granted I cannot hijack the frontend to do arithmetics but that's not something that cannot be solved either with an extra method with 3 instead of 2 parameters. Given the fact that I was able to solve the subpar codegen just promoting the local used for constructing IntPtr to long it suggest that is something that a Roslyn performance analyzer could hint or the JIT could solve altogether promoting the local type.