On Wed, Sep 19, 2018 at 7:07 AM Fabien COELHO <coe...@cri.ensmp.fr> wrote: > > I reviewed 'pgbench-prp-func/pgbench-prp-func-4.patch'. [...] I thinks > > this patch is fine.
modular_multiply() is an interesting device. I will leave this to committers with a stronger mathematical background than me, but I have a small comment in passing: +#ifdef HAVE__BUILTIN_POPCOUNTLL + return __builtin_popcountll(n); +#else /* use clever no branching bitwise operator version */ I think it is not enough for the compiler to have __builtin_popcountll(). The CPU that this is eventually executed on must actually have the POPCNT instruction[1] (or equivalent on ARM, POWER etc), or the program will die with SIGILL. There are CPUs in circulation produced in this decade that don't have it. I have previously considered something like this[2], but realised you would therefore need a runtime check. There are a couple of ways to do that: see commit a7a73875 for one example, also __builtin_cpu_supports(), and direct CPU ID bit checks (some platforms). There is also the GCC "multiversion" system that takes care of that for you but that worked only for C++ last time I checked. [1] https://en.wikipedia.org/wiki/SSE4#POPCNT_and_LZCNT [2] https://www.postgresql.org/message-id/CAEepm%3D3g1_fjJGp38QGv%2B38BC2HHVkzUq6s69nk3mWLgPHqC3A%40mail.gmail.com -- Thomas Munro http://www.enterprisedb.com
