gh-Cyan4973/xxHash

Watch

Fork 0

XXH3 and Windows ARM builds #220

New issue

Closed

opened 2019-06-26 19:05:58 -05:00 by carstenskyboxlabs · 6 comments

carstenskyboxlabs commented

2019-06-26 19:05:58 -05:00

(Migrated from github.com)

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.

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](https://docs.microsoft.com/en-us/cpp/intrinsics/emul-emulu?view=vs-2019)). ``` /* 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.

👍 2

Cyan4973 commented

2019-06-26 19:21:16 -05:00

(Migrated from github.com)

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.

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](https://docs.microsoft.com/en-us/cpp/intrinsics/emul-emulu?view=vs-2019). 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](https://ci.appveyor.com/project/YannCollet/xxhash). It would have caught this issue, and potential future ones.

carstenskyboxlabs commented

2019-06-26 19:54:30 -05:00

(Migrated from github.com)

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.

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

2019-06-27 16:21:06 -05:00

(Migrated from github.com)

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

cd cmake_unofficial
mkdir build
cd build
cmake -G "Visual Studio 16 2019" -A ARM ..
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
  )

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 ) ```

👍 2

Cyan4973 commented

2019-06-27 16:53:20 -05:00

(Migrated from github.com)

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

easyaspi314 commented

2019-07-03 16:07:47 -05:00

(Migrated from github.com)

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

This should probably work. It should fix ARM compilation, and it should [improve x64 performance](https://godbolt.org/z/gqPr9v) ```c #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 ```

👍 1

Cyan4973 commented

2019-07-03 17:39:09 -05:00

(Migrated from github.com)

Fixed in dev branch

Fixed in `dev` branch

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Cyan4973/xxHash#220

No description provided.

Rows
Columns