Avoid redefinition of _bit_scan_{forward,reverse} macros

[amadio]

Fixes #247.

[Axel-Naumann]

Apple clang has /Library/Developer/CommandLineTools//usr/lib/clang/11.0.0/include/x86intrin.h. Shouldn't we use that instead of entering the #elif branch here? I.e. is #if Vc_GCC >= 0x40500 really correct, or should it be #if Vc_GCC >= 0x40500 || defined(Vc_CLANG) || defined(Vc_APPLECLANG)?

[amadio]

I thought about this, it's the other option. I just don't know which versions of clang have or don't have the macro. Maybe generically defining it if it's not defined is better than both?

[amadio]

The point is that even if the header exists, sometimes the macro is not defined. So we can include the header and if the macro is not defined, we define it.

[mattkretz]

Right, but what happens if x86intrin.h defines the macro but wasn't included yet? This header would still define the macro and a subsequent #include <x86intrin.h> leads to an error?
I think the right fix is to unconditionally include x86intrin.h for GCC, Clang and AppleClang before testing !defined(_bit_scan_forward).

[amadio]

Yes, I agree. I will update the change. There were a couple of failures in Travis probably due to that. BTW, do you plan to make a new patch release of 1.4 anytime soon?

[mattkretz]

Thank you for the PR! See my comment inline

[mattkretz]

Right, but what happens if x86intrin.h defines the macro but wasn't included yet? This header would still define the macro and a subsequent #include <x86intrin.h> leads to an error?
I think the right fix is to unconditionally include x86intrin.h for GCC, Clang and AppleClang before testing !defined(_bit_scan_forward).

[amadio]

How's this version? I tried to run tests, but ran into

[ 10%] Building CXX object tests/CMakeFiles/logarithm_avx2.dir/logarithm.cpp.o
In file included from /Users/sftnight/amadio/Vc/tests/logarithm.cpp:28:
/Users/sftnight/amadio/Vc/tests/unittest.h:30:10: fatal error: 'virtest/vir/test.h' file not found
#include "virtest/vir/test.h"
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.

which I don't know how to fix. What's providing this header? I guess it's an external dependency, but CMake configured without errors, so I'm confused.

[amadio]

Hi @mattkretz, I cannot see what the error was, so I don't know if it's related to my changes. Can you please let me know?

[Axel-Naumann]

Ping, @mattkretz please! :-)

[Axel-Naumann]

@amadio Google says it's https://github.com/mattkretz/virtest - could you try with that, please?

[hahnjo]

How's this version? I tried to run tests, but ran into

[ 10%] Building CXX object tests/CMakeFiles/logarithm_avx2.dir/logarithm.cpp.o
In file included from /Users/sftnight/amadio/Vc/tests/logarithm.cpp:28:
/Users/sftnight/amadio/Vc/tests/unittest.h:30:10: fatal error: 'virtest/vir/test.h' file not found
#include "virtest/vir/test.h"
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.

which I don't know how to fix. What's providing this header? I guess it's an external dependency, but CMake configured without errors, so I'm confused.

The header comes from a submodule (of the repo that Axel posted), the README mentions git submodule update --init.

After doing this, I was able to run the tests and all pass with this change (GCC 8.3.1 and Clang 9.0.1 from CentOS 8) while making the build with Clang free of warnings. Can we merge this one? @mattkretz

[agheata]

Can we merge this now?

[mattkretz]

Looking at it now. Probably yes. :-)

[mattkretz]

Can we use C++20? 😜
The bsr inline asm for clang is scary. I'm sure we can use a __builtin nowadays. The PR seems to be a strict improvement, but I'd rather rewrite this header altogether. I'll merge it for now and might push a cleanup later.