gmp icon indicating copy to clipboard operation
gmp copied to clipboard

Erroneous results from mpz_popcount()

Open VulcanForge opened this issue 10 months ago • 3 comments

Context

I encounter erroneous, unpredictable results from the mpz_popcount() function. I have included a minimal C++ program to demonstrate the behaviour below:

#include <iostream>

#include <gmpxx.h>

int main ()
{
    mpz_t n;
    mpz_init_set_ui (n, 65535);
    std::cout << mpz_popcount (n);
    mpz_clear (n);
    return 0;
}

Expected Behavior

As 65535 is 2^16 - 1, it contains 16 1's in its binary representation, and mpz_popcount(n) should return 16.

Actual Behavior

On five consecutive runs, mpz_popcount(n) returns 160, 146, 168, 137, and 141.

Environment

  • CPU: AMD Ryzen 7 7730U (Zen3 architecture)
  • Operating System and Version: Windows 11 Home, 22631.3447
  • Compiler and Version(s): Microsoft (R) C/C++ Optimizing Compiler Version 19.39.33523 for x86
  • Visual Studio 2022, version 17.9.6 with VSYASM

Possible Fix

I am guessing that the error lies in the assembly file SMP/mpn/x86_64/popcount.s. Since the function output is not consistent across runs, I suspect that the assembly code is reading data past the end of the mpz_t. However, I am not familiar with assembly, so I do not have a specific suggestion for a fix. I compared to the GMP repo on their official site, and did not find a matching implementation of mpn_popcount, so I think this bug is specific to the ShiftMediaProject fork.

VulcanForge avatar Apr 23 '24 04:04 VulcanForge

The popcount assembly file is just generated from "https://github.com/ShiftMediaProject/gmp/blob/master/mpn/x86_64/core2/popcount.asm" by manually triggering the same conversions that the stock gmp build tools use to generate assembly files (just a preprocessor). You can check #11 for details as to how they are generated and could be replaced by other versions (you may want to try changing it with the coreinhm fileversion and test that). If other assembly functions have similar errors then there might be an issue in the assembler itself if it's just popcount then trying a different variant as suggested before may fix the issue)

Sibras avatar Apr 23 '24 08:04 Sibras

I changed it to the "zen" version instead of "coreinhm" because I have an AMD CPU, and it fixed the problem. Thank you! Do you think that the "core2" assembly file has a bug, or is it a case of hardware incompatibility, or something else? I will report it to the GMP team if you think it is the first case.

VulcanForge avatar Apr 25 '24 13:04 VulcanForge

Looking into it it apears that the original source of popcount may not be compatible with msvc's calling convention. Ive changed the default assembly source for popcount (and a couple of others) which should correct this issue.

Sibras avatar Apr 27 '24 15:04 Sibras

I forgot to close the issue, but your last commit fixed the problem. Thank you!

VulcanForge avatar May 18 '24 16:05 VulcanForge