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

fix #2017 #2073

Merged
merged 1 commit into from
Dec 6, 2024
Merged

fix #2017 #2073

merged 1 commit into from
Dec 6, 2024

Conversation

quantresearch1
Copy link

@quantresearch1 quantresearch1 commented Dec 5, 2024

@galabovaa The issue mentions a SIGILL when running on older x86_64 CPUs such as a Core 2 Duo. I will defer to your expertise but I am wondering if we can get away with checking compiler support only for the POPCNT instruction ? While this doesn't do runtime CPU detection, in practice modern compilers are pretty good at handling CPU compatibility issues. If the flag is supported but the CPU doesn't support the instruction, most compilers will generate appropriate fallback code.

If you think this is not sufficient, I am happy to add something like this to detect cpu support:

    if(COMPILER_SUPPORTS_POPCNT)
        check_cxx_source_runs("
            #include <stdint.h>
            #if defined(_MSC_VER)
                #include <intrin.h>
                #define popcnt64 __popcnt64
            #else
                #include <x86intrin.h>
                #define popcnt64 _mm_popcnt_u64
            #endif
            int main() {
                uint64_t x = 0xAAAAAAAA55555555ULL;
                return popcnt64(x) == 32;
            }"
            CPU_SUPPORTS_POPCNT)
    endif()

Also, I think this message is slightly misleading for two reasons:

if (WIN32)
    message("FLAG_MPOPCNT_SUPPORTED is not available on this architecture")
  1. The -mpopcnt flag is specific to GCC/Clang compilers, on Windows, MSVC doesn't use these kinds of flags. However, the check would wrongly skip the -mpopcnt flag even when using MinGW or Clang on Windows, which do support it.
  2. check_cxx_compiler_flag() will automatically return false for MSVC when checking -mpopcnt

@jajhall jajhall changed the base branch from master to latest December 6, 2024 11:32
@jajhall jajhall requested a review from galabovaa December 6, 2024 11:32
@galabovaa
Copy link
Contributor

At a first glance it looks good to me! I need to look at it a bit further next week, to see why the PDLP solve of one test problem gives a different output after this change.

@jajhall
Copy link
Member

jajhall commented Dec 6, 2024

At a first glance it looks good to me! I need to look at it a bit further next week, to see why the PDLP solve of one test problem gives a different output after this change.

These occasional failures (for 25fv47) resulted in me dropping (in latest) the last "8" in its objective function used in the test for WIN32 (as it had been for APPLE) in the pdlpInstances for CI. However, this "8" is still in master, so the CI tests will fail for PRs built from branches of master. Once merged into latest, they will pass. I saw this with #2067: once I'd merged into a local branch, and then merged latest into it, the CI tests passed.

Another way of putting it is that master wouldn't pass our CI tests, now that they include instances solved by PDLP

@jajhall jajhall merged commit c92942d into ERGO-Code:latest Dec 6, 2024
101 of 104 checks passed
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

Successfully merging this pull request may close these issues.

3 participants