cc @tannergooding

@danmosemsft

Well @john-h-k perhaps you might consider adding a benchmark (such as some form of the one you're using here) to https://github.com/dotnet/performance as well ? There's excellent documentation there: it's easy to use. That would protect your change.

Looking at that right now - will make sure to do so

The SSE pathway doesn't spill to the stack, unlike the others

The spill that happens on the non-SSE path is something that can be fixed in the JIT. I'm not sure if once that is done the SSE path would still be significantly faster to warrant the extra complexity as the ANDPS/ORPS instructions have lower throughput compared to the their integer counterparts.

For benchmarking purposes it would be useful to test the non-SSE version with SingleToInt64Bits replaced by SSE's MOVD intrinsic.

For benchmarking purposes it would be useful to test the non-SSE version with SingleToInt64Bits replaced by SSE's MOVD intrinsic.

Though with ANDNPS I suspect that there's no way that the non-SSE version can be faster, even with MOVD. The SSE code with ANDNPS is:

       48B80000000000000080 mov      rax, 0x8000000000000000
       C4E1F96ED0           vmovd    xmm2, rax
       C5E854C9             vandps   xmm1, xmm2, xmm1
       C5E855C0             vandnps  xmm0, xmm2, xmm0
       C5F856C1             vorps    xmm0, xmm0, xmm1

which is probably as good as it gets.

just for reference here is what mono generates for:

 [MethodImpl(MethodImplOptions.NoInlining)]
 public static double Test(double x, double y) => Math.CopySign(x, y);

asm:

0000000000000000	movabsq	$0x7fb20121f3e0, %rax
000000000000000a	vandps	(%rax), %xmm1, %xmm1
000000000000000e	movabsq	$0x7fb20121f3f0, %rax
0000000000000018	vandps	(%rax), %xmm0, %xmm0
000000000000001c	vorps	%xmm1, %xmm0, %xmm0
0000000000000020	retq

(llvm.copysign.f64)

you might consider adding a benchmark

That would be greatly appreciated and would make validating this change on other boxes simpler 😄. In general, the benchmarks are being checked into the dotnet/performance repo. -- @adamsitnik, do we have any guidance on how to run dotnet/performance benchmarks against local dotnet/coreclr builds?

Overall the changes LGTM, just want to test this on a few more boxes (and probably an ARM machine) before committing it.

Testing on sharplab, the JIT doesn't seem to inline this method in its current state, but will with aggressiveinlining applied. Is it worth applying it to the method given the discussion around its use in tight loops

which I believe could be impactful to tight loops.

The SSE double version is problematic on x86:

       C5FB10442444 vmovsd   xmm0, qword ptr [esp+44H]
       C5F911442420 vmovupd  xmmword ptr [esp+20H], xmm0
       C5FB104C243C vmovsd   xmm1, qword ptr [esp+3CH]
       C5F9114C2410 vmovupd  xmmword ptr [esp+10H], xmm1
       6800000080   push     0x80000000
       6A00         push     0
       8D4C2408     lea      ecx, bword ptr [esp+08H]
       FF1514C08E05 call     [System.Runtime.Intrinsics.Vector128:CreateScalarUnsafe(long):struct]
       C5F9100424   vmovupd  xmm0, xmmword ptr [esp]
       C5F855442420 vandnps  xmm0, xmm0, xmmword ptr [esp+20H]
       C5F9104C2410 vmovupd  xmm1, xmmword ptr [esp+10H]
       C5F0540C24   vandps   xmm1, xmm1, xmmword ptr [esp]
       C5F856C1     vorps    xmm0, xmm0, xmm1
       C5FB11442430 vmovsd   qword ptr [esp+30H], xmm0
       DD442430     fld      qword ptr [esp+30H]

It probably should only be enabled on x64.

It probably should only be enabled on x64.

Sounds good to me.

It's worth noting that some of that looks like codegen issues we might want to track... There isn't really any reason why this couldn't be:

vmovq  xmm2, qword ptr [address of ulong constant]  ; This encoding of movq is supported even on 32-bit
vandps xmm1, xmm2, xmm1
vandnps xmm0, xmm2, xmm0
vorps  xmm0, xmm0, xmm1

do we have any guidance on how to run dotnet/performance benchmarks against local dotnet/coreclr builds?

yes, all you need to do is to pass the path to corerun executable via --corerun parameter

https://github.com/dotnet/performance/tree/master/src/benchmarks/micro#private-runtime-builds

@john-h-k please let me know if something is not clear or does not work as expected

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

1. In your coreclr repository clone, create patch by running git format-patch origin
2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.