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

add a test combining Windows + Visual + Clang-Cl + DISPATCH=1 #863

Merged
merged 1 commit into from
Jul 15, 2023

Conversation

Cyan4973
Copy link
Owner

test that #763 is fixed and remains fixed

@t-mat
Copy link
Contributor

t-mat commented Jul 15, 2023

https://github.com/Cyan4973/xxHash/actions/runs/5562735680/jobs/10161223314?pr=863#step:7:15

Since we don't see the "Enable xxHash dispatch mode" message from the following block, it seems it doesn't work properly.

if("${PLATFORM}" STREQUAL "x86_64")
message(STATUS "Enable xxHash dispatch mode")
add_library(xxhash "${XXHASH_DIR}/xxh_x86dispatch.c"
"${XXHASH_DIR}/xxhash.c"
)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DXXHSUM_DISPATCH=1")
else()

@Cyan4973
Copy link
Owner Author

Cyan4973 commented Jul 15, 2023

mmmh, that doesn't work.
The generated xxhsum binary still doesn't use DISPATCH despite setting -DDISPATCH=1 at cmake build time.

Strangely, the same command seems to work fine in local tests on Ubuntu.
So maybe this could be related to the Windows + Visual Studio environment (?)

@t-mat
Copy link
Contributor

t-mat commented Jul 15, 2023

It seems the actual Architecture (${PLATFORM}) for MSVC x86_64 is AMD64

https://github.com/Cyan4973/xxHash/actions/runs/5562735680/jobs/10161223314?pr=863#step:7:22

-- Detecting C compile features - done
-- Architecture: AMD64
-- Configuring done (2.5s)

The following patch for cmake_unofficial/CMakeList.txt enables dispatch mode for clang-cl.

# Only support DISPATCH option on x86_64.
- if("${PLATFORM}" STREQUAL "x86_64")
+ if(("${PLATFORM}" STREQUAL "x86_64") OR ("${PLATFORM}" STREQUAL "AMD64"))

But it may cause error. See also #763 (comment)

@Cyan4973 Cyan4973 force-pushed the visual_clang_dispatch_test branch from 730caf1 to 22d0948 Compare July 15, 2023 15:12
@t-mat t-mat mentioned this pull request Jul 15, 2023
@t-mat
Copy link
Contributor

t-mat commented Jul 15, 2023

After #865, this PR should work properly.

@Cyan4973 Cyan4973 force-pushed the visual_clang_dispatch_test branch from 22d0948 to de57565 Compare July 15, 2023 20:31
@Cyan4973
Copy link
Owner Author

rebasing on top of @t-mat 's #865

@Cyan4973 Cyan4973 force-pushed the visual_clang_dispatch_test branch from de57565 to a8ded18 Compare July 15, 2023 20:36
@Cyan4973
Copy link
Owner Author

This works great ! Thanks @t-mat !

The test will now also properly fail with a clear error signal if the produced xxhsum binary is not autoVec.

@t-mat
Copy link
Contributor

t-mat commented Jul 15, 2023

https://github.com/Cyan4973/xxHash/actions/runs/5564095301/jobs/10163482759#step:7:25

-- Enable xxHash dispatch mode

https://github.com/Cyan4973/xxHash/actions/runs/5564095444/jobs/10163482094?pr=863#step:8:7

compiled as 64-bit x86_64 autoVec little endian with Clang 12.0.0  

🥳

@Cyan4973 Cyan4973 merged commit 0c8e930 into dev Jul 15, 2023
@Cyan4973 Cyan4973 deleted the visual_clang_dispatch_test branch July 20, 2023 21:07
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