[ADT] Allow K to be incomplete during DenseMap<K*, V> instantiation

[rnk]

Summary

DenseMap requires two sentinel values for keys: empty and tombstone
values. To avoid undefined behavior, LLVM aligns the two sentinel
pointers to alignof(T). This requires T to be complete, which is
needlessly restrictive.

Instead, assume that DenseMap pointer keys have a maximum alignment of
4096, and use the same sentinel values for all pointer keys. The new
sentinels are:

empty:     static_cast<uintptr_t>(-1) << 12
tombstone: static_cast<uintptr_t>(-2) << 12

These correspond to the addresses of -4096 and -8192. Hopefully, such a
key is never inserted into a DenseMap.

I encountered this while looking at making clang's SourceManager not
require FileManager.h, but it has several maps keyed on classes defined
in FileManager.h. FileManager depends on various LLVM FS headers, which
cumulatively take ~200ms to parse, and are generally not needed.

[hans]

These correspond to the addresses of -4096 and -8192. Hopefully, such a key is never inserted into a DenseMap.

Maybe we could have an assert somewhere which checks that an empty or tombstone marker is never inserted by the user?

[rnk]

Yes, DenseMap already asserts if you try to lookup or insert the two sentinel keys. My comment had more to do with -4096 being a more plausible pointer value than -16/-8, so we are marginally more likely to hit that assert, maybe in a kernel context, like opencl drivers.

llvm/include/llvm/ADT/DenseMapInfo.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ADT_DENSEMAPINFO_H
 #define LLVM_ADT_DENSEMAPINFO_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/PointerLikeTypeTraits.h"
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <utility>
 
 namespace llvm {
 
 template<typename T>
 struct DenseMapInfo {
   //static inline T getEmptyKey();
   //static inline T getTombstoneKey();
   //static unsigned getHashValue(const T &Val);
   //static bool isEqual(const T &LHS, const T &RHS);
 };
 
-// Provide DenseMapInfo for all pointers.
+// Provide DenseMapInfo for all pointers. Come up with sentinel pointer values
+// that are aligned to alignof(T) bytes, but try to avoid requiring T to be
+// complete. This allows clients to instantiate DenseMap<T*, ...> with forward
+// declared key types. Assume that no pointer key type requires more than 4096
+// bytes of alignment.
 template<typename T>
 struct DenseMapInfo<T*> {
+  // The following should hold, but it would require T to be complete:
+  // static_assert(alignof(T) <= (1 << Log2MaxAlign),
+  //               "DenseMap does not support pointer keys requiring more than "
+  //               "Log2MaxAlign bits of alignment");
+  static constexpr uintptr_t Log2MaxAlign = 12;
+
   static inline T* getEmptyKey() {
     uintptr_t Val = static_cast<uintptr_t>(-1);
-    Val <<= PointerLikeTypeTraits<T*>::NumLowBitsAvailable;
+    Val <<= Log2MaxAlign;
     return reinterpret_cast<T*>(Val);
   }
 
   static inline T* getTombstoneKey() {
     uintptr_t Val = static_cast<uintptr_t>(-2);
-    Val <<= PointerLikeTypeTraits<T*>::NumLowBitsAvailable;
+    Val <<= Log2MaxAlign;
     return reinterpret_cast<T*>(Val);
   }
 
   static unsigned getHashValue(const T *PtrVal) {
     return (unsigned((uintptr_t)PtrVal) >> 4) ^
            (unsigned((uintptr_t)PtrVal) >> 9);
   }
 

llvm/unittests/ADT/DenseMapTest.cpp

TEST(DenseMapCustomTest, ConstTest) {
   int *B = &A;
   const int *C = &A;
   Map.insert({B, 0});
   EXPECT_EQ(Map.count(B), 1u);
   EXPECT_EQ(Map.count(C), 1u);
   EXPECT_NE(Map.find(B), Map.end());
   EXPECT_NE(Map.find(C), Map.end());
 }
+
+struct IncompleteStruct;
+
+TEST(DenseMapCustomTest, OpaquePointerKey) {
+  // Test that we can use a pointer to an incomplete type as a DenseMap key.
+  // This is an important build time optimization, since many classes have
+  // DenseMap members.
+  DenseMap<IncompleteStruct *, int> Map;
+  int Keys[3] = {0, 0, 0};
+  IncompleteStruct *K1 = reinterpret_cast<IncompleteStruct *>(&Keys[0]);
+  IncompleteStruct *K2 = reinterpret_cast<IncompleteStruct *>(&Keys[1]);
+  IncompleteStruct *K3 = reinterpret_cast<IncompleteStruct *>(&Keys[2]);
+  Map.insert({K1, 1});
+  Map.insert({K2, 2});
+  Map.insert({K3, 3});
+  EXPECT_EQ(Map.count(K1), 1u);
+  EXPECT_EQ(Map[K1], 1);
+  EXPECT_EQ(Map[K2], 2);
+  EXPECT_EQ(Map[K3], 3);
+  Map.clear();
+  EXPECT_EQ(Map.find(K1), Map.end());
+  EXPECT_EQ(Map.find(K2), Map.end());
+  EXPECT_EQ(Map.find(K3), Map.end());
+}
 }