Skip to content

[DenseMap] Update combineHashValue #95970

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

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jun 18, 2024

combineHashValue is a custom bit mixer from 2008
(5fc8ab6) used for std::pair and
std::tuple. It has a long dependency chain and slow. Replace it with
a simply multiply-xorshift style hash using a constant from
splitmix641. abseil-cpp and carbon also use this style, but with
uint128 to probably get a lower avalanche bias. We don't use uint128 for
MSVC portability.

Measured time to compute [0,1000000000) values on an i7-11850H:

  • old: 1.163s
  • new: 0.427s

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Member

llvmbot commented Jun 18, 2024

@llvm/pr-subscribers-llvm-adt

Author: Fangrui Song (MaskRay)

Changes

combineHashValue is a custom bit mixer from 2008
(5fc8ab6) used for std::pair and
std::tuple. It has a long dependency chain and slow. Replace it with
Moremur from Pelle Evensen1

Measured time to compute [0,1000000000) values on an i7-11850H:

  • old: 1.152s
  • new: 0.769s

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

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/DenseMapInfo.h (+8-10)
diff --git a/llvm/include/llvm/ADT/DenseMapInfo.h b/llvm/include/llvm/ADT/DenseMapInfo.h
index 5b7dce7b53c62..ba05309e4e413 100644
--- a/llvm/include/llvm/ADT/DenseMapInfo.h
+++ b/llvm/include/llvm/ADT/DenseMapInfo.h
@@ -26,17 +26,15 @@ namespace llvm {
 namespace detail {
 
 /// Simplistic combination of 32-bit hash values into 32-bit hash values.
+/// This uses a Murmur3-type mixer "Moremur" from Pelle Evensen.
 static inline unsigned combineHashValue(unsigned a, unsigned b) {
-  uint64_t key = (uint64_t)a << 32 | (uint64_t)b;
-  key += ~(key << 32);
-  key ^= (key >> 22);
-  key += ~(key << 13);
-  key ^= (key >> 8);
-  key += (key << 3);
-  key ^= (key >> 15);
-  key += ~(key << 27);
-  key ^= (key >> 31);
-  return (unsigned)key;
+  uint64_t x = (uint64_t)a << 32 | (uint64_t)b;
+  x ^= x >> 27;
+  x *= 0x3C79AC492BA7B653UL;
+  x ^= x >> 33;
+  x *= 0x1C69B3F74AC4AE35UL;
+  x ^= x >> 27;
+  return (unsigned)x;
 }
 
 } // end namespace detail

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

MaskRay added a commit that referenced this pull request Jun 19, 2024
DenseMap iteration order is not guaranteed to be deterministic.

Without the change,
llvm/test/Transforms/CodeGenPrepare/X86/statepoint-relocate.ll would
fail when `combineHashValue` changes (#95970).

Fixes: dba7329
Created using spr 1.3.5-bogner
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

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

LGTM.

Created using spr 1.3.5-bogner
MaskRay added a commit that referenced this pull request Jun 19, 2024
DenseMap iteration order is not guaranteed to be deterministic.

Without the change,
llvm/test/Transforms/GlobalMerge/basic.ll could fail when
`combineHashValue` changes (#95970).
Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit fb17bbc into main Jun 19, 2024
4 of 6 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/densemap-update-combinehashvalue branch June 19, 2024 17:30
MaskRay added a commit that referenced this pull request Jun 19, 2024
DenseMap iteration order is not guaranteed to be deterministic.

Without the change, clang/test/Driver/linker-wrapper{,-libs}.c would
fail when `combineHashValue` changes (#95970).
@MaskRay
Copy link
Member Author

MaskRay commented Jun 20, 2024

Significant instruction:u difference for -O3 builds. This commit itself affects std::pair.

https://llvm-compile-time-tracker.com/compare.php?from=58d7a6e0e6361871442df956bb88798ce602b09d&to=fb17bbce80cf76ce1a31eff463f451f626bc36b5&stat=instructions:u

stage2-O3:

Benchmark Old New
kimwitu++ 39227M 39106M (-0.31%)
sqlite3 35441M 35309M (-0.37%)
consumer-typeset 32091M 32021M (-0.22%)
Bullet 93517M 93231M (-0.31%)
tramp3d-v4 79022M 78706M (-0.40%)
mafft 33162M 33040M (-0.37%)
ClamAV 50706M 50535M (-0.34%)
lencod 61560M 61298M (-0.42%)
SPASS 43262M 43080M (-0.42%)
7zip 192562M 192060M (-0.26%)
geomean 55639M 55449M (-0.34%)

After #95734 and af82e63 , DenseMap<unsigned long, X> and DenseMap<unsigned long long, X> are affected as well.

@kazutakahirata
Copy link
Contributor

Significant instruction:u difference for -O3 builds.

Very nice!

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
DenseMap iteration order is not guaranteed to be deterministic.

Without the change,
llvm/test/Transforms/CodeGenPrepare/X86/statepoint-relocate.ll would
fail when `combineHashValue` changes (llvm#95970).

Fixes: dba7329
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
DenseMap iteration order is not guaranteed to be deterministic.

Without the change,
llvm/test/Transforms/GlobalMerge/basic.ll could fail when
`combineHashValue` changes (llvm#95970).
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
`combineHashValue` is a custom bit mixer from 2008
(5fc8ab6) used for std::pair and
std::tuple. It has a long dependency chain and slow. Replace it with
a simply multiply-xorshift style hash using a constant from
splitmix64[1]. abseil-cpp and carbon also use this style, but with
uint128 to probably get a lower avalanche bias. We don't use uint128 for
MSVC portability.

Measured time to compute [0,1000000000) values on an i7-11850H:

* old: 1.163s
* new: 0.427s

[1]: https://jonkagstrom.com/tuning-bit-mixers/index.html

Pull Request: llvm#95970
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
DenseMap iteration order is not guaranteed to be deterministic.

Without the change, clang/test/Driver/linker-wrapper{,-libs}.c would
fail when `combineHashValue` changes (llvm#95970).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants