[KeyInstr] MDNodeKeyImpl<DILocation> skip zero values for hash #143357
Conversation
Hashing AtomGroup and AtomRank substantially impacts performance whether Key Instructions is enabled or not. We can't detect whether it's enabled here cheaply; avoiding hashing zero values is a good approximation. This affects Key Instruction builds too, but any potential costs incurred by messing with the hash distribution (hash_combine(x) != hash_combine(x, 0)) appear to still be massively outweighed by the overall compile time savings by performing this check.
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-debuginfo Author: Orlando Cazalet-Hyams (OCHyams) ChangesHashing AtomGroup and AtomRank substantially impacts performance whether Key Instructions is enabled or not. We can't detect whether it's enabled here cheaply; avoiding hashing zero values is a good approximation. This affects Key Instruction builds too, but any potential costs incurred by messing with the hash distribution (hash_combine(x) != hash_combine(x, 0)) appear to still be massively outweighed by the overall compile time savings by performing this check. From compile-time-tracker:
Compare 2-1: https://llvm-compile-time-tracker.com/compare.php?from=882faf0ac573476edf0f4026a69f6f86f316c821&to=3fa3a147a0d7a579f92f5ca6db1bfcdd485f8ffa&stat=instructions%3Au Full diff: https://github.com/llvm/llvm-project/pull/143357.diff 1 Files Affected:
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index 21f5c06ea24f3..7b6083a7a3496 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -355,13 +355,19 @@ template <> struct MDNodeKeyImpl<DILocation> {
}
unsigned getHashValue() const {
- return hash_combine(Line, Column, Scope, InlinedAt, ImplicitCode
#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
- ,
- AtomGroup, (uint8_t)AtomRank);
-#else
- );
+ // Hashing AtomGroup and AtomRank substantially impacts performance whether
+ // Key Instructions is enabled or not. We can't detect whether it's enabled
+ // here cheaply; avoiding hashing zero values is a good approximation. This
+ // affects Key Instruction builds too, but any potential costs incurred by
+ // messing with the hash distribution* appear to still be massively
+ // outweighed by the overall compile time savings by performing this check.
+ // * (hash_combine(x) != hash_combine(x, 0))
+ if (AtomGroup || AtomRank)
+ return hash_combine(Line, Column, Scope, InlinedAt, ImplicitCode,
+ AtomGroup, (uint8_t)AtomRank);
#endif
+ return hash_combine(Line, Column, Scope, InlinedAt, ImplicitCode);
}
};
|
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
I think the main problem is that you are going from hashing 25 bytes to 34, which means you go up one hash function (> 32). I think if you packed ImplicitCode, AtomGroup and AtomRank into one uint64_t value for the hash, performance would be about unchanged from not hashing them? (Reducing AtomGroup to 60 bits.)
|
I hadn't thought of that, thanks. I tried reducing it to the 32 byte hash by reducing (note the comparison is 32 bytes against this patch as a base). |
|
Interesting! What I find peculiar is how big the difference between stage1-O0-g and stage2-O0-g is. It seems like Clang is doing something extremely terrible here compared to GCC, and it would be nice to find out what that is and whether we can fix it... |
Yeah, the difference is quite extreme. I've put that on my wishlist to look at if/when I get a quiet day, if no one beats me to it. Thanks for the review |
|
Might as well make the u16 column change while I'm here - #143399 |
…143357) [KeyInstr] MDNodeKeyImpl<DILocation> skip zero values for hash Hashing AtomGroup and AtomRank substantially impacts performance whether Key Instructions is enabled or not. We can't detect whether it's enabled here cheaply; avoiding hashing zero values is a good approximation. This affects Key Instruction builds too, but any potential costs incurred by messing with the hash distribution (hash_combine(x) != hash_combine(x, 0)) appear to still be massively outweighed by the overall compile time savings by performing this check. See PR for compile-time-tracker numbers.
3 participants
Hashing AtomGroup and AtomRank substantially impacts performance whether Key Instructions is enabled or not. We can't detect whether it's enabled here cheaply; avoiding hashing zero values is a good approximation. This affects Key Instruction builds too, but any potential costs incurred by messing with the hash distribution (hash_combine(x) != hash_combine(x, 0)) appear to still be massively outweighed by the overall compile time savings by performing this check.
From compile-time-tracker:
Compare 2-1: https://llvm-compile-time-tracker.com/compare.php?from=882faf0ac573476edf0f4026a69f6f86f316c821&to=3fa3a147a0d7a579f92f5ca6db1bfcdd485f8ffa&stat=instructions%3Au
Compare 3-2: https://llvm-compile-time-tracker.com/compare.php?from=54d544b83141dc0b20727673f68793728ed54793&to=882faf0ac573476edf0f4026a69f6f86f316c821&stat=instructions%3Au