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 xxh3_128bits for Win32 builds after #95863 #96931

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jun 27, 2024

__emulu is used without including intrin.h. Actually, it's better to
rely on compiler optimizations. In this LLVM copy, we try to eliminate
unneceeded workarounds for old compilers.

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 27, 2024

@llvm/pr-subscribers-llvm-support

Author: Fangrui Song (MaskRay)

Changes

__emulu is used without including intrin.h. Actually, it's better to
rely on compiler optimizations. In this LLVM copy, we try to eliminate
unneceeded workarounds for old compilers.


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

1 Files Affected:

  • (modified) llvm/lib/Support/xxhash.cpp (-11)
diff --git a/llvm/lib/Support/xxhash.cpp b/llvm/lib/Support/xxhash.cpp
index a0803297555ce..607789b391381 100644
--- a/llvm/lib/Support/xxhash.cpp
+++ b/llvm/lib/Support/xxhash.cpp
@@ -453,18 +453,7 @@ uint64_t llvm::xxh3_64bits(ArrayRef<uint8_t> data) {
 #define XXH_rotl64(x, r) (((x) << (r)) | ((x) >> (64 - (r))))
 #endif
 
-#if defined(_MSC_VER) && defined(_M_IX86)
-#define XXH_mult32to64(x, y) __emulu((unsigned)(x), (unsigned)(y))
-#else
-/*
- * Downcast + upcast is usually better than masking on older compilers like
- * GCC 4.2 (especially 32-bit ones), all without affecting newer compilers.
- *
- * The other method, (x & 0xFFFFFFFF) * (y & 0xFFFFFFFF), will AND both operands
- * and perform a full 64x64 multiply -- entirely redundant on 32-bit.
- */
 #define XXH_mult32to64(x, y) ((uint64_t)(uint32_t)(x) * (uint64_t)(uint32_t)(y))
-#endif
 
 /*!
  * @brief Calculates a 64->128-bit long multiply.

@MaskRay MaskRay changed the title [Support] Fix xxh3_128bits for Win32 builds [Support] Fix xxh3_128bits for Win32 builds after #95863 Jun 27, 2024
Copy link
Collaborator

@dstutt dstutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me

@MaskRay MaskRay merged commit 7205562 into main Jun 28, 2024
9 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/support-fix-xxh3_128bits-for-win32-builds branch June 28, 2024 07:19
@dukebw
Copy link
Contributor

dukebw commented Jun 29, 2024

Thank you for the fix! For my education: is there a workflow I can monitor that would catch this regression?

@MaskRay
Copy link
Member Author

MaskRay commented Jul 2, 2024

Thank you for the fix! For my education: is there a workflow I can monitor that would catch this regression?

IIUC there is no bot for 32-bit Windows. In addition, Windows 11 has removed 32-bit support.
Therefore, the condition #if defined(_MSC_VER) && defined(_M_IX86) makes the failure unlikely caught by build bots. I am actually curious how you noticed the breakage due to xxh3_128bits :)

lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
`__emulu` is used without including `intrin.h`. Actually, it's better to
rely on compiler optimizations. In this LLVM copy, we try to eliminate
unneceeded workarounds for old compilers.

Pull Request: llvm#96931
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.

4 participants