[PATCH] D55585: RFC: [LLD][COFF] Parallel GHASH generation at link-time -- NOT FOR SUBMIT

[Leonardo Santagada via Phabricator via llvm-commits]

santagada added inline comments.


================
Comment at: llvm/trunk/include/llvm/DebugInfo/CodeView/GlobalTypeDenseMap.h:77
+          typename BucketT = GloballyHashedInfo>
+class GlobalTypeDenseMap : public DebugEpochBase {
+  template <typename T>
----------------
aganea wrote:
> rnk wrote:
> > I haven't read this implementation yet, it's quite long, but broadly I'm in favor of having a custom hash table here. This is the most performance critical thing LLD does.
> > 
> > This seems like a good separable change, since this map isn't actually used in parallel.
> It's mostly a copy of `DenseMap`, but without tombstones, and with a different `DenseHashInfo` API.
> I was wondering if a insert-only `DenseMap` would be useful in other parts of LLVM/Clang?
I know this patch is old, but I was reading it again and I don't understand how you reconstruct the key if there were a collision on the bucket while inserting... as the bucketno will not be the same bits that were stripped when the key got packed.

In other words: LookupBucketFor starts looking for an empty bucket at the extracted bits from the key, but might have probed ahead of that number by the time we insert it in the bucket. At that time it seems that any other searches for the key on the map will fail as the key computed from that bucket doesn't have the correct value were the bucketmask is.

Also bucketmask needs to be all the lower bits of key, else there is information loss there.

Or maybe I'm just very confused on how this packing of having half the key as bucketno and half in the KV value is happening.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55585/new/

https://reviews.llvm.org/D55585