secp256k1 icon indicating copy to clipboard operation
secp256k1 copied to clipboard

WIP WIP WIP 5x64 field representation

Open sipa opened this issue 2 years ago • 2 comments

This swaps out the 5x52 field with a 5x64 one, including both inline and external x86_64 asm code (by @kn-cs).

I'm just opening this to see if it doesn't break anything on the various platforms we have.

sipa avatar Jul 24 '21 00:07 sipa

So to give an idea of the status here:

  • The code works, is generally a speedup, and is unlikely to change much; if it does, it's probably restricted to just better asm code
  • There is an issue on macOS to figure out (linking fails; I wonder if the asm/linker detect platform-specific instructions, and need some kind of magic runtime detection annotation?)
  • As is, this PR removes the old 5x52 representation. That may be undesirable, as at least on some (uncommon) platforms, the new code is (slightly) slower. This may be even more the case with #810 merged now.
  • It's an open question how this is supposed to be reviewed or otherwise gained confidence in. I know there are formal methods of proving semantics of asm code, but I don't have experience with them. An alternative is adding lots of high-level description to the asm code to make it readable.
  • This PR leaves the decision on which repr/impl to use up to the ./configure step. That's useful in some settings, and enough to enable testing, but not what we want for normal production usage. I'm fine with that, but perhaps people want a clearer plan on how runtime autodetection etc would work before proceeding?

sipa avatar Oct 17 '21 19:10 sipa

There is an issue on macOS to figure out (linking fails; I wonder if the asm/linker detect platform-specific instructions, and need some kind of magic runtime detection annotation?)

According to the CI output, it's the assembler that fails because it does not like the .type directive. A shot in the dark is to simply remove them on MacOS (indicators: https://github.com/briansmith/ring/issues/312#issuecomment-274581871 and https://code.woboq.org/llvm/compiler-rt/lib/builtins/assembly.h.html). I think if you rename .s to .S you can use the C preprocessor with things like #ifdef __APPLE__.

real-or-random avatar Oct 18 '21 12:10 real-or-random