[DenseMap] Optimize find/erase #100517
Merged
[DenseMap] Optimize find/erase #100517
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.5-bogner
|
@llvm/pr-subscribers-llvm-adt Author: Fangrui Song (MaskRay) Changes
Full diff: https://github.com/llvm/llvm-project/pull/100517.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 7ccc9445c0a7b..e130861307ac9 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -143,8 +143,7 @@ class DenseMapBase : public DebugEpochBase {
/// Return true if the specified key is in the map, false otherwise.
bool contains(const_arg_type_t<KeyT> Val) const {
- const BucketT *TheBucket;
- return LookupBucketFor(Val, TheBucket);
+ return doFind(Val) != nullptr;
}
/// Return 1 if the specified key is in the map, 0 otherwise.
@@ -153,21 +152,17 @@ class DenseMapBase : public DebugEpochBase {
}
iterator find(const_arg_type_t<KeyT> Val) {
- BucketT *TheBucket;
- if (LookupBucketFor(Val, TheBucket))
- return makeIterator(TheBucket,
- shouldReverseIterate<KeyT>() ? getBuckets()
- : getBucketsEnd(),
- *this, true);
+ if (BucketT *Bucket = doFind(Val))
+ return makeIterator(
+ Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
+ *this, true);
return end();
}
const_iterator find(const_arg_type_t<KeyT> Val) const {
- const BucketT *TheBucket;
- if (LookupBucketFor(Val, TheBucket))
- return makeConstIterator(TheBucket,
- shouldReverseIterate<KeyT>() ? getBuckets()
- : getBucketsEnd(),
- *this, true);
+ if (const BucketT *Bucket = doFind(Val))
+ return makeConstIterator(
+ Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
+ *this, true);
return end();
}
@@ -178,31 +173,26 @@ class DenseMapBase : public DebugEpochBase {
/// type used.
template<class LookupKeyT>
iterator find_as(const LookupKeyT &Val) {
- BucketT *TheBucket;
- if (LookupBucketFor(Val, TheBucket))
- return makeIterator(TheBucket,
- shouldReverseIterate<KeyT>() ? getBuckets()
- : getBucketsEnd(),
- *this, true);
+ if (BucketT *Bucket = doFind(Val))
+ return makeIterator(
+ Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
+ *this, true);
return end();
}
template<class LookupKeyT>
const_iterator find_as(const LookupKeyT &Val) const {
- const BucketT *TheBucket;
- if (LookupBucketFor(Val, TheBucket))
- return makeConstIterator(TheBucket,
- shouldReverseIterate<KeyT>() ? getBuckets()
- : getBucketsEnd(),
- *this, true);
+ if (const BucketT *Bucket = doFind(Val))
+ return makeConstIterator(
+ Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
+ *this, true);
return end();
}
/// lookup - Return the entry for the specified key, or a default
/// constructed value if no such entry exists.
ValueT lookup(const_arg_type_t<KeyT> Val) const {
- const BucketT *TheBucket;
- if (LookupBucketFor(Val, TheBucket))
- return TheBucket->getSecond();
+ if (const BucketT *Bucket = doFind(Val))
+ return Bucket->getSecond();
return ValueT();
}
@@ -643,6 +633,34 @@ class DenseMapBase : public DebugEpochBase {
return TheBucket;
}
+ template <typename LookupKeyT> BucketT *doFind(const LookupKeyT &Val) {
+ BucketT *BucketsPtr = getBuckets();
+ const unsigned NumBuckets = getNumBuckets();
+ if (NumBuckets == 0)
+ return nullptr;
+
+ const KeyT EmptyKey = getEmptyKey();
+ unsigned BucketNo = getHashValue(Val) & (NumBuckets - 1);
+ unsigned ProbeAmt = 1;
+ while (true) {
+ BucketT *Bucket = BucketsPtr + BucketNo;
+ if (LLVM_LIKELY(KeyInfoT::isEqual(Val, Bucket->getFirst())))
+ return Bucket;
+ if (LLVM_LIKELY(KeyInfoT::isEqual(Bucket->getFirst(), EmptyKey)))
+ return nullptr;
+
+ // Otherwise, it's a hash collision or a tombstone, continue quadratic
+ // probing.
+ BucketNo += ProbeAmt++;
+ BucketNo &= NumBuckets - 1;
+ }
+ }
+
+ template <typename LookupKeyT>
+ const BucketT *doFind(const LookupKeyT &Val) const {
+ return const_cast<DenseMapBase *>(this)->doFind(Val); // NOLINT
+ }
+
/// LookupBucketFor - Lookup the appropriate bucket for Val, returning it in
/// FoundBucket. If the bucket contains the key and a value, this returns
/// true, otherwise it returns a bucket with an empty marker or tombstone and
|
|
stage2-O3:
|
|
lgtm, nice improvement. I think erase could also use doFind. |
Nice catch! Added support for |
aengelke
approved these changes
Jul 25, 2024
|
Hello @MaskRay! Somehow this commit (and after) is failing one clang test when building the Windows LLVM package. The problem does not occur on the previous commit |
Hi! The failure looks weird. (a) Quite a few Windows bots are happy with this change. (b) If there were an issue, I suspect that quite a few tests would fail as well. I think figuring this out might require the assistance of a debugger? Do you have luck with that ? :) |
|
Yes I will debug it. This is with latest MSVC, it could be very well a UB triggered by this test. |
|
@MaskRay @aganea We are seeing the same test failure on our windows build of LLVM. Only that one test is failing, and a git bisect lead back to this PR. It only fails when running with a release build. A debug build does not hit the failure. We are building with |
MaskRay
added a commit
that referenced
this pull request
Aug 8, 2024
Port the ADT optimization #100517. In addition, add `contains` (https://reviews.llvm.org/D145895) while changing `count`. Pull Request: #101785
|
This is caused by bad codegen in latest MSVC. I'm using latest version available: As seen in the previous message, the symptom is that a bad alignement is generated in one of the load instructions: When
The code on the right is translated to the assembly on the left. It's a tad scary, but just shuffling around the lines of code in |
aganea
added a commit
that referenced
this pull request
Aug 10, 2024
…#102681) Before this PR, when using the latest MSVC `Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33813 for x64` one of the Clang unit test used to fail: `CodeGenObjC/gnustep2-direct-method.m`, see full failure log: [here](#100517 (comment)). This PR temporarily shuffles around the code to make the MSVC inliner/ optimizer happy and avoid the bug. MSVC bug report: https://developercommunity.visualstudio.com/t/Bad-code-generation-when-building-LLVM-w/10719589?port=1025&fsid=e572244a-cde7-4d75-a73d-9b8cd94204dd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LookupBucketForis used forfind,insert,erase, and theirvariants. While tombstone comparison isn't needed by
find/erase,LookupBucketForcallsgetTombstoneKeyregardless, which might havean opaque implementation or just not optimized out, leading to
unnecessary overhead for
findanderase.