Improve hash for DenseMapInfo::getHashValue()

[higher-performance]

Proposing this on behalf of a colleague. I don't have public benchmarks here, but they mentioned they got an ~8.1x improvement in the performance of getParent through this hash improvement.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: None (higher-performance)

Changes

Proposing this on behalf of a colleague. I don't have public benchmarks here, but they mentioned they got an ~8.1x improvement in the performance of getParent through this hash improvement.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/ADT/DenseMapInfo.h (+5-2)

diff --git a/llvm/include/llvm/ADT/DenseMapInfo.h b/llvm/include/llvm/ADT/DenseMapInfo.h
index 07c37e353a40b..8eb74a7793e26 100644
--- a/llvm/include/llvm/ADT/DenseMapInfo.h
+++ b/llvm/include/llvm/ADT/DenseMapInfo.h
@@ -82,8 +82,11 @@ struct DenseMapInfo<T*> {
   }
 
   static unsigned getHashValue(const T *PtrVal) {
-    return (unsigned((uintptr_t)PtrVal) >> 4) ^
-           (unsigned((uintptr_t)PtrVal) >> 9);
+    uintptr_t Val = (uintptr_t)PtrVal;
+    uint32_t Rot = static_cast<uint32_t>(Val);
+    Rot = (Rot >> 9) | (Rot << (32 - 9));
+    Val += Rot;
+    return densemap::detail::mix(Val);
   }
 
   static bool isEqual(const T *LHS, const T *RHS) { return LHS == RHS; }

[nikic]

This is mostly a regression: https://llvm-compile-time-tracker.com/compare.php?from=aa27d4e0c3aef8047828aa453f2943730aa779c6&to=cbb73001f447b2de4f5478f0d304065b6b22ad75&stat=instructions:u

The one outlier is 7zip, which sometimes sees an improvement.

Proposing this on behalf of a colleague. I don't have public benchmarks here, but they mentioned they got an ~8.1x improvement in the performance of getParent through this hash improvement.

What getParent is this referring to?

[higher-performance]

Oh interesting, thanks for sharing that. I believe they were referring to ParentMapContext::getParents, in AST matching.

I wonder if we could customize the hasher for that in that case...

[nikic]

I wonder if we could customize the hasher for that in that case...

DenseMap accepts an explicit DenseMapInfo parameter to override the default, so that is possible.

I'm curious why that particular case benefits though. Presumably the current hash causes a lot of collisions, but from a cursory look at the code it's not obvious what's special about ParentMap.