secp256k1: ASM implementation of Field64 on AMD64 using MULX/ADX

[valery-osheter-cb]

Summary

• Add amd64 MULX/ADX assembly for FieldVal64 mul/square
• Split portable 512-bit primitives into field64_generic.go behind a purego || !amd64 build tag.
• Add self-contained CPUID detection for MULX/ADX in field64_amd64.go.

[davecgh]

Thank you very much for the PR. I will start taking a closer look at the asm in depth soon.

A couple of things I noticed in the mean time:

1. Please add tests similar to how blake256 does it where it tests as many of the implementations (generic and specialized) as possible depending on the available hardware support when the tests run. (e.g. https://github.com/decred/dcrd/blob/master/crypto/blake256/internal/compress/blocks_amd64_test.go#L20)
2. Please provide comparative benchmarks for the generic versus specialized implementation. I don't have an Intel available at the moment and I'd prefer results form real hardware over emulation.

[davecgh]

Something definitely looks off with this. Mul, and Square now allocate and it's causing a significant slowdown versus the generic.

Benchmarks from a Ryzen 5800X3D:

$ benchstat beforeadx.txt afteradx.txt
name              old time/op    new time/op    delta
Field64MulInt    20.6ns ± 3%    38.4ns ± 4%   +86.26%  (p=0.000 n=9+10)
Field64Mul       18.6ns ± 3%    32.7ns ± 5%   +75.81%  (p=0.000 n=9+10)
Field64Square    13.7ns ± 5%    29.5ns ± 4%  +115.17%  (p=0.000 n=10+9)

name              old alloc/op   new alloc/op   delta
Field64MulInt     0.00B         32.00B ± 0%     +Inf%  (p=0.000 n=10+10)
Field64Mul        0.00B         32.00B ± 0%     +Inf%  (p=0.000 n=10+10)
Field64Square     0.00B         32.00B ± 0%     +Inf%  (p=0.000 n=10+10)

name              old allocs/op  new allocs/op  delta
Field64MulInt      0.00           1.00 ± 0%     +Inf%  (p=0.000 n=10+10)
Field64Mul         0.00           1.00 ± 0%     +Inf%  (p=0.000 n=10+10)
Field64Square      0.00           1.00 ± 0%     +Inf%  (p=0.000 n=10+10)

[davecgh]

Something definitely looks off with this. Mul, and Square now allocate and it's causing a significant slowdown versus the generic.

After looking at it more closely, it's the field64Mul and field64Square closures caused by e.g. pickField64Mul. Defining func field64Mul(r *[4]uint64, a, b *[4]uint64) { directly with the selection logic defined in the func itself no longer escapes to the heap and thus avoids the alloc.

With that change (i didn't update square, but it should be the same thing):

name           old time/op    new time/op    delta
Field64Mul    18.6ns ± 3%    13.8ns ± 9%  -25.90%  (p=0.000 n=9+9)

name           old alloc/op   new alloc/op   delta
Field64Mul     0.00B          0.00B          ~     (all equal)

name           old allocs/op  new allocs/op  delta
Field64Mul      0.00           0.00          ~     (all equal)

This is why I requested benchmarks!

[davecgh]

I've manually reviewed the asm for field64MulADX up through the REDUCE macro. I wanted to go ahead and give some feedback up to this point. I will continue reviewing the rest later.

[davecgh]

Commenting for future reviewers that this is safe because the XORQ BX, BX above unconditionally clears CF (and OF for the ADOX call below) per the Intel Software Developer's Manual (and AMD's) despite many other sources claiming that it only affects ZF and SF and leaves CF and OF unmodified.

[davecgh]

Alright, I finished reviewing all of the assembly and asserting correctness and making sure all carries are propagated properly through both independent CF and OF chains.

Very nice work overall!

I did notice in field64SquareADX that you could likely ever-so-slightly improve scheduling since MULX takes 3-4 cycles per agner fog's tables and you have three of them back-to-back when you could technically interleave them with the ADCX like you did in field64MulADX. That said, I doubt it would be very much difference, if any, and it's very easy to follow and prove correctness as it is, which I find valuable. So, I don't think it's worth changing it.

Based on my previous blake256 work, something I do highly suspect would be ~5-10% faster is to use the memory operands directly versus putting them into XMM registers and then having to extract the upper 64 bits from them via PEXTR. e.g.

MOVQ   a+8(FP), SI
XORQ   R13, R13
XORQ   R15, R15
XORQ   CX, CX

// Off-diagonal upper-triangle products into p1..p6 (a1->SI, a3->DI reused).
MOVQ   0(SI), DX
MOVQ   24(SI), DI
MOVQ   16(SI), R8
MOVQ   8(SI), SI
...

I know that theoretically dealing with the registers should be faster, but, my benchmarks consistently have shown that the above tends to win out when it's only a single load like this. Perhaps it is specific to the micro-architecture.

[davecgh]

I know that theoretically dealing with the registers should be faster, but, my benchmarks consistently have shown that the above tends to win out when it's only a single load like this. Perhaps it is specific to the micro-architecture.

Out of curiosity, I went ahead and made that change and benchmarked it. It appears that the pattern holds:

$ benchstat squareadx_xmm.txt squareadx_memops.txt

name             old time/op    new time/op    delta
Field64Square    11.0ns ± 1%    10.1ns ± 2%  -7.51%  (p=0.000 n=10+10)

[valery-osheter-cb]

I did the fixes:

• XMM is removed, so SSE4.1 is not required
• Benchmark is added. It compares generic to MULX/ADX

I also don't have Intel hardware, my PC is M3, so I use CI for the tests. I did the branch field64-amd64-CI for that.
For the correctness tests, can normal Field64 tests be used instead of dedicated AMD64 tests? With optional purego?

[davecgh]

Thanks for the updates. I've reviewed the updated assembly and reasserted correctness with the updates. I've also confirmed the fix for closures so it is no longer allocating.

Here is a comparative benchmark from your original XMM implementation and the new direct load implementation for mul on a zen4. It's an even bigger increase to perf than I suspected it would be for mul. I only expected ~5-10% and instead it's ~10-15%.

name              old time/op    new time/op    delta
Field64Mul       14.4ns ± 4%    12.5ns ± 3%  -13.08%  (p=0.000 n=10+9)

I am quite curious to know how it compares on Intel though, since these results are for AMD (it's similar for earlier generations of zen as well, btw). It's quite possible Intel handles the scheduling differently which could potentially make it more or less efficient, but I would not at all be surprised if this current implementation is also notably faster on Intel. It is essentially trading out 18 instructions that involve larger instructions to decode and extra deps between the SIMD and integer register files, for what is certainly going to be hot loads from half an L1 cache line. I would also expect it to be less work for the scheduler, because it also gets rid of the uops for register allocation and writeback.

For your tests question, that's fine. I can also make any modifications later. I'm mostly concerned with there being tests in place at the time of merge to prevent any possible regressions in the mean time.