[clang] Properly cache member pointer LLVM types

[aeubanks]

Summary

When not going through the main Clang->LLVM type cache, we'd
accidentally create multiple different opaque types for a member pointer
type.

This allows us to remove the -verify-type-cache flag now that
check-clang passes with it on. We can do the verification in expensive
builds. Previously microsoft-abi-member-pointers.cpp was failing with
-verify-type-cache.

I suspect that there may be more issues when we have multiple member
pointer types and we clear the cache, but we can leave that for later.

Followup to D118744.

[aeubanks]

update

[rnk]

Seems reasonable

clang/lib/CodeGen/CodeGenTypes.cpp

776

Can this use something like the insert get-or-create pattern?

auto Insertion = RecordsWithOpaqueMemberPointers.insert({C, StructType*{}});
if (Insertion.second) Insertion.first->second = llvm::StructType::create(...);
return Insertion.first->second;

[aeubanks]

update

[rnk]

lgtm

clang/lib/CodeGen/CodeGenTypes.h

class CodeGenTypes {
   bool SkippedLayout;
 
   SmallVector<const RecordDecl *, 8> DeferredRecords;
 
   /// This map keeps cache of llvm::Types and maps clang::Type to
   /// corresponding llvm::Type.
   llvm::DenseMap<const Type *, llvm::Type *> TypeCache;
 
-  llvm::SmallSet<const Type *, 8> RecordsWithOpaqueMemberPointers;
+  llvm::DenseMap<const Type *, llvm::Type *> RecordsWithOpaqueMemberPointers;
 
   static constexpr unsigned FunctionInfosLog2InitSize = 9;
   /// Helper for ConvertType.
   llvm::Type *ConvertFunctionTypeInternal(QualType FT);
 
 public:
   CodeGenTypes(CodeGenModule &cgm);
   ~CodeGenTypes();

clang/lib/CodeGen/CodeGenTypes.cpp

 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Module.h"
 
 using namespace clang;
 using namespace CodeGen;
 
-#ifndef NDEBUG
-#include "llvm/Support/CommandLine.h"
-// TODO: turn on by default when defined(EXPENSIVE_CHECKS) once check-clang is
-// -verify-type-cache clean.
-static llvm::cl::opt<bool> VerifyTypeCache(
-    "verify-type-cache",
-    llvm::cl::desc("Verify that the type cache matches the computed type"),
-    llvm::cl::init(false), llvm::cl::Hidden);
-#endif
-
 CodeGenTypes::CodeGenTypes(CodeGenModule &cgm)
   : CGM(cgm), Context(cgm.getContext()), TheModule(cgm.getModule()),
     Target(cgm.getTarget()), TheCXXABI(cgm.getCXXABI()),
     TheABIInfo(cgm.getTargetCodeGenInfo().getABIInfo()) {
   SkippedLayout = false;
 }
 
 CodeGenTypes::~CodeGenTypes() {
llvm::Type *CodeGenTypes::ConvertType(QualType T) {
   bool ShouldUseCache =
       Ty->isBuiltinType() ||
       (noRecordsBeingLaidOut() && FunctionsBeingProcessed.empty());
   if (ShouldUseCache) {
     llvm::DenseMap<const Type *, llvm::Type *>::iterator TCI =
         TypeCache.find(Ty);
     if (TCI != TypeCache.end())
       CachedType = TCI->second;
-    if (CachedType) {
-#ifndef NDEBUG
-      if (!VerifyTypeCache)
-        return CachedType;
-#else
+      // With expensive checks, check that the type we compute matches the
+      // cached type.
+#ifndef EXPENSIVE_CHECKS
+    if (CachedType)
       return CachedType;
 #endif
-    }
-  }
 
   // If we don't have it in the cache, convert it now.
   llvm::Type *ResultType = nullptr;
   switch (Ty->getTypeClass()) {
   case Type::Record: // Handled above.
 #define TYPE(Class, Base)
 #define ABSTRACT_TYPE(Class, Base)
 #define NON_CANONICAL_TYPE(Class, Base) case Type::Class:
case Type::BlockPointer: {
     unsigned AS = Context.getTargetAddressSpace(FTy.getQualifiers());
     ResultType = llvm::PointerType::get(PointeeType, AS);
     break;
   }
 
   case Type::MemberPointer: {
     auto *MPTy = cast<MemberPointerType>(Ty);
     if (!getCXXABI().isMemberPointerConvertible(MPTy)) {
-      RecordsWithOpaqueMemberPointers.insert(MPTy->getClass());
-      ResultType = llvm::StructType::create(getLLVMContext());
+      auto *C = MPTy->getClass();
+      auto Insertion = RecordsWithOpaqueMemberPointers.insert({C, nullptr});
+      if (Insertion.second)
+        Insertion.first->second = llvm::StructType::create(getLLVMContext());
+      ResultType = Insertion.first->second;
     } else {
       ResultType = getCXXABI().ConvertMemberPointerType(MPTy);
     }
     break;
   }
 
   case Type::Atomic: {
     QualType valueType = cast<AtomicType>(Ty)->getValueType();
#include "clang/AST/BuiltinTypes.def"
   case Type::BitInt: {
     const auto &EIT = cast<BitIntType>(Ty);
     ResultType = llvm::Type::getIntNTy(getLLVMContext(), EIT->getNumBits());
     break;
   }
   }
 
   assert(ResultType && "Didn't convert a type?");
-
-#ifndef NDEBUG
-  if (CachedType) {
-    assert(CachedType == ResultType &&
-           "Cached type doesn't match computed type");
-  }
-#endif
+  assert((!CachedType || CachedType == ResultType) &&
 
   if (ShouldUseCache)
     TypeCache[Ty] = ResultType;
   return ResultType;
 }
 
 bool CodeGenModule::isPaddedAtomicType(QualType type) {
   return isPaddedAtomicType(type->castAs<AtomicType>());

clang/test/CodeGenCXX/type-cache-2.cpp

-// RUN: %clang_cc1 -mllvm -verify-type-cache -emit-llvm %s -o - -triple i386-pc-windows-msvc19.16.0 | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple i386-pc-windows-msvc19.16.0 | FileCheck %s
 // REQUIRES: asserts, x86-registered-target
 
 // CHECK: call void @"?dc@z@@SAXU1@@Z"
 struct z {
   static void dc(z);
   void (*p)(z);
 };
 
 void f() {
   z::dc({});
 }

clang/test/CodeGenCXX/type-cache-3.cpp

-// RUN: %clang_cc1 -mllvm -verify-type-cache -emit-llvm %s -o - -triple i386-pc-windows-msvc19.16.0 | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple i386-pc-windows-msvc19.16.0 | FileCheck %s
 // REQUIRES: asserts, x86-registered-target
 
 // CHECK-LABEL: define {{.*}}@"?f@@YAXXZ"(
 // CHECK: call void @"?dc@z@@SAXU1@@Z"
 
 // CHECK-LABEL: define {{.*}}@"?dc@z@@SAXU1@@Z"(
 // CHECK: store void ({}*)* %{{.*}}, void ({}*)** %{{.*}}
 struct z {
   static void dc(z) {}
   void (*p)(z);
 };
 
 void f() {
   z::dc({});
 }

clang/test/CodeGenCXX/type-cache.cpp

-// RUN: %clang_cc1 -mllvm -verify-type-cache -emit-llvm %s -o - -triple i386-pc-windows-msvc19.16.0 | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple i386-pc-windows-msvc19.16.0 | FileCheck %s
 // REQUIRES: asserts, x86-registered-target
 
 // CHECK: call {}* @"?f@@YA?AUz@@XZ"()
 
 struct z {
   z (*p)();
 };
 
 z f();
 
 void g() {
   f();
 }