search

fplll / g6k

The General Sieve Kernel
GNU General Public License v2.0
99 stars 30 forks source link

Stability changes to SIMD code (building for Clang + on the M1). #108

Open joerowell opened 2 years ago

joerowell commented 2 years ago

TL;DR: This PR makes some small structural changes to the SIMD code. The code works as before, but it's slightly closer to standard C++. Might also be worth considering if we want to test how fast a pure C++ version of the bucketer is. Big thanks to both @ElenaKirshanova and @malb for helping me to debug some of these issues and fixing some of the build code respectively.

This PR fixes the SIMD code on Clang and ARM machines. It turns out that some of the SIMD code didn't build compile with Clang or crashed on the M1. This meant that some changes had to be made to e.g the shuffling code. There were also some type-punning differences that I had to fix (see all of the extra calls to memcpy).

Note that this PR doesn't add full support for G6K to the M1. All of the sieves work, except for the HK3 sieve in a multi-threaded setting (there's a use-after-free crash). This is something we hope to fix.

As before, the tests for this code are here.

Note that as part of implementing this, I had to re-implement every Intel intrinsic that we use in standard C++ (so that I could test the intrinsics on ARM). This means that we could switch over to a pure C++ implementation of the bucketer if needed: we just have to switch out the calls to the intrinsics.

Finally, there's one outstanding curiousity: the vectorised random number code (that I wrote) sometimes gets stuck in a fixed point. I just replaced this with two calls to rand, and I didn't notice any performance differences. This might be something to look into.

ElenaKirshanova commented 2 years ago

Tested on M1 with Martin's setup.py from https://github.com/fplll/g6k/pull/107 All compiles well.

lducas commented 9 months ago

Any reason we haven't merged this yet ??

joerowell commented 9 months ago

Afaik, no one ever reviewed it. However, since it's a bit old now, I'll rebase with the most recent Cython changes and see if it still works.

On Sat, 2 Dec 2023, 06:12 Léo Ducas, @.***> wrote:

Any reason we haven't merged this yet ??

— Reply to this email directly, view it on GitHub https://github.com/fplll/g6k/pull/108#issuecomment-1837056242, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7CASGC7DLYU3ACGWNDJ4LYHLBDBAVCNFSM53Z4DK42U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBTG4YDKNRSGQZA . You are receiving this because you authored the thread.Message ID: @.***>