[PATCH] D52283: [PDB] Add the ability to resolve forward references

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/TpiHashing.h:60
+    codeview::UnionRecord Union;
+  };
+
----------------
You will probably have to name the union field for this to build with all supported compilers.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/TpiStream.h:87
 
+  std::vector<std::vector<codeview::TypeIndex>> HashMap;
+
----------------
Nested C++ containers don't usually perform the best. Here come some obnoxious data structure micro-optimization suggestions down below!



================
Comment at: llvm/lib/DebugInfo/PDB/Native/TpiStream.cpp:146
 
+void TpiStream::buildHashMap() {
+  if (!HashMap.empty())
----------------
How about this:
1. Allocate an array of TypeIndex of size `TypeIndexEnd - TypeIndexBegin`, i.e. an array of all the type indices in the hash table.
2. Make HashMap a `std::vector<std::pair<unsigned, unsigned>>`, where these are begin/end indices of ranges into the previous array. We can write a helper that takes a hash value and returns an `ArrayRef<TypeIndex>` using these two arrays.
3. Build a temporary array of pairs of HashValue+TypeIndex, and sort it by hash value.
4. Iterate over the sorted array of hash values and type indices. Push each type index onto the array. When the hash value changes, store a pair of begin/end indices for the type indices that share this hash code into the HashMap.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/TpiStream.cpp:157
+  while (TIB < TIE) {
+    uint32_t HV = HashValues[TIB.toArrayIndex()];
+    HashMap[HV].push_back(TIB++);
----------------
If `HV >= Header->NumHashBuckets`, I would report an error or just drop the type. We could hash the type ourselves as a recovery, but it's a corrupt input file, anything but UB or crashing works.


https://reviews.llvm.org/D52283