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

Hide avx512 function definitions from headers #579

Closed
wants to merge 3 commits into from

Conversation

lemire
Copy link
Member

@lemire lemire commented Jan 29, 2024

The very latest version of Visual Studio appears to turn pure AVX code into AVX-512 binaries if the function, at any point could run AVX-512 instructions. This cause a regression for us.

We have code that looks like this...

int bitset_container_compute_cardinality(const bitset_container_t *bitset) {
    int support = croaring_hardware_support();
#if CROARING_COMPILER_SUPPORTS_AVX512
    if( support & ROARING_SUPPORTS_AVX512 ) {
      return (int) avx512_vpopcount(
        (const __m512i *)bitset->words,
        BITSET_CONTAINER_SIZE_IN_WORDS / (WORDS_IN_AVX512_REG));
    } else
#endif // CROARING_COMPILER_SUPPORTS_AVX512
    if( support & ROARING_SUPPORTS_AVX2 ) {
      return (int) avx2_harley_seal_popcount256(
        (const __m256i *)bitset->words,
        BITSET_CONTAINER_SIZE_IN_WORDS / (WORDS_IN_AVX2_REG));
    } else {
      return _scalar_bitset_container_compute_cardinality(bitset);

    }
}

This is likely fine as long as avx512_vpopcount does not get inlined into bitset_container_compute_cardinality. But clearly, if avx512_vpopcount gets inline, it makes it likely that Visual Studio might decide to just go AVX-512 all in.

This PR makes avx512_vpopcount and related functions regular (non-inline) functions. Hopefully, this is sufficient.

Reference: RoaringBitmap/croaring-rs#128

@Dr-Emann
Copy link
Member

Dr-Emann commented Feb 1, 2024

Suggesting we close this, there doesn't appear to be any amount of hiding the AVX512 code we can do to fix it, because the bug doesn't require any AVX512 source code at all: RoaringBitmap/croaring-rs#128 (comment)

@lemire lemire closed this Feb 26, 2024
@lemire lemire reopened this Mar 1, 2024
@lemire lemire closed this Mar 1, 2024
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.

2 participants