Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Erroneous results from mpz_popcount() #17

Closed
VulcanForge opened this issue Apr 23, 2024 · 4 comments
Closed

Erroneous results from mpz_popcount() #17

VulcanForge opened this issue Apr 23, 2024 · 4 comments

Comments

@VulcanForge
Copy link

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.

@Sibras
Copy link
Member

Sibras commented Apr 23, 2024

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)

@VulcanForge
Copy link
Author

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.

@Sibras
Copy link
Member

Sibras commented Apr 27, 2024

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.

@VulcanForge
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants