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

[Support] Fix llvm::xxh3_128bits for win 32 builds #96891

Closed
wants to merge 1 commit into from

Conversation

dstutt
Copy link
Collaborator

@dstutt dstutt commented Jun 27, 2024

#95863 caused build failures for Win32 builds.
This change includes the necessary include to fix it.

llvm#95863 caused build failures for Win32 builds.
This change includes the necessary include to fix it.
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 27, 2024

@llvm/pr-subscribers-llvm-support

Author: David Stuttard (dstutt)

Changes

#95863 caused build failures for Win32 builds.
This change includes the necessary include to fix it.


Full diff: https://github.com/llvm/llvm-project/pull/96891.diff

1 Files Affected:

  • (modified) llvm/lib/Support/xxhash.cpp (+3)
diff --git a/llvm/lib/Support/xxhash.cpp b/llvm/lib/Support/xxhash.cpp
index a0803297555ce..8e1763809bf01 100644
--- a/llvm/lib/Support/xxhash.cpp
+++ b/llvm/lib/Support/xxhash.cpp
@@ -46,6 +46,9 @@
 #include "llvm/Support/Endian.h"
 
 #include <stdlib.h>
+#if defined(_MSC_VER) && defined(_M_IX86)
+#include <intrin.h>
+#endif
 
 using namespace llvm;
 using namespace support;

@dstutt dstutt requested a review from dukebw June 27, 2024 10:26
@dstutt
Copy link
Collaborator Author

dstutt commented Jun 27, 2024

I'm not sure why this only affected win32 for us - but including the header resolved the problem.

@dstutt dstutt requested a review from MaskRay June 27, 2024 10:30
@MaskRay
Copy link
Member

MaskRay commented Jun 27, 2024

Can you edit the first comment (will be used as the commit message) to say that __emulu needs <intrin.h>`?

Upstream xxhash has a lot of code like this. Some are obviously unnecessary, but retained to work around certain very old compilers. Our simplified xxhash.cpp shouldn't have such workarounds.

@MaskRay
Copy link
Member

MaskRay commented Jun 27, 2024

Actually, I think we should remove __emulu: 96931

@dstutt
Copy link
Collaborator Author

dstutt commented Jun 28, 2024

Closing in favour of #96931

@dstutt dstutt closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants