Cyan4973 / xxHash

Extremely fast non-cryptographic hash algorithm
http://www.xxhash.com/
Other
8.97k stars 769 forks source link

XXH3 and Windows ARM builds #220

Closed carstenskyboxlabs closed 5 years ago

carstenskyboxlabs commented 5 years ago

I have a problem with using xxh3.h on Widows while building for an ARM architecture. The problem is that __emulu is an x64/x86 only windows internal macro (MS Docs).

/* U64 XXH_mult32to64(U32 a, U64 b) { return (U64)a * (U64)b; } */
#ifdef _MSC_VER
#   include <intrin.h>
    /* MSVC doesn't do a good job with the mull detection. */
#   define XXH_mult32to64 __emulu
#else
#   define XXH_mult32to64(x, y) ((U64)((x) & 0xFFFFFFFF) * (U64)((y) & 0xFFFFFFFF))
#endif

Where it's defined only checks for an _MSC_VER before using it. It would be very helpful if this could check architecture as well. Say either by not including it when ARM or only using it on x64/x86 processors. Thanks.

Cyan4973 commented 5 years ago

Good point, thanks for notification @carstenskyboxlabs !

We should likely enforce that this code path is only valid for x86/x64, since that's the target where __emulu instruction is defined.

Side note : it would be great if I could find a way to test Visual Studio compilation for ARM, and automate such a test into AppveyorCI. It would have caught this issue, and potential future ones.

carstenskyboxlabs commented 5 years ago

Thanks for the response. I'm not sure I've never used AppveyorCI, but I do know that VS/MSBuild does support UWP ARM builds.

CaptainCMake commented 5 years ago

When I run these steps using CMake and Visual Studio 2019:

  1. cd cmake_unofficial
  2. mkdir build
  3. cd build
  4. cmake -G "Visual Studio 16 2019" -A ARM ..
  5. cmake --build .

I get this error:

xxhash.obj : error LNK2019: unresolved external symbol __emulu referenced in function XXH3_mul128_fold64 [C:\Users\steve\Do
cuments\xxHash\cmake_unofficial\build\xxhash.vcxproj]
C:\Users\steve\Documents\xxHash\cmake_unofficial\build\Debug\xxhash.dll : fatal error LNK1120: 1 unresolved externals [C:\U
sers\steve\Documents\xxHash\cmake_unofficial\build\xxhash.vcxproj]

I don't have much experience with appveyor.yml, but following the existing pattern, I think this will let you generate and run for ARM:

- if "%PLATFORM%"=="visual_arm" (
      cd cmake_unofficial &&
      cmake . -DCMAKE_BUILD_TYPE=Release -A ARM &&
      cmake --build . --config Release
  )
Cyan4973 commented 5 years ago

Thanks @v-strob for the hint ! Worth trying !

easyaspi314 commented 5 years ago

This should probably work. It should fix ARM compilation, and it should improve x64 performance

#if defined(_MSC_VER) && defined(_M_IX86)
#    include <intrin.h>
#    define XXH_mult32to64 __emulu
#else
#    define XXH_mult32to64(x, y) ((U64)((x) & 0xFFFFFFFF) * (U64)((y) & 0xFFFFFFFF))
#endif
Cyan4973 commented 5 years ago

Fixed in dev branch