[PATCH] D119215: [clang] Properly cache member pointer LLVM types

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
aeubanks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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 <https://reviews.llvm.org/D118744>.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119215

Files:
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/CodeGenTypes.h
  clang/test/CodeGenCXX/type-cache-2.cpp
  clang/test/CodeGenCXX/type-cache-3.cpp
  clang/test/CodeGenCXX/type-cache.cpp


Index: clang/test/CodeGenCXX/type-cache.cpp
===================================================================
--- clang/test/CodeGenCXX/type-cache.cpp
+++ clang/test/CodeGenCXX/type-cache.cpp
@@ -1,4 +1,4 @@
-// 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"()
Index: clang/test/CodeGenCXX/type-cache-3.cpp
===================================================================
--- clang/test/CodeGenCXX/type-cache-3.cpp
+++ clang/test/CodeGenCXX/type-cache-3.cpp
@@ -1,4 +1,4 @@
-// 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"(
Index: clang/test/CodeGenCXX/type-cache-2.cpp
===================================================================
--- clang/test/CodeGenCXX/type-cache-2.cpp
+++ clang/test/CodeGenCXX/type-cache-2.cpp
@@ -1,4 +1,4 @@
-// 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"
Index: clang/lib/CodeGen/CodeGenTypes.h
===================================================================
--- clang/lib/CodeGen/CodeGenTypes.h
+++ clang/lib/CodeGen/CodeGenTypes.h
@@ -96,7 +96,7 @@
   /// corresponding llvm::Type.
   llvm::DenseMap<const Type *, llvm::Type *> TypeCache;
 
-  llvm::SmallSet<const Type *, 8> RecordsWithOpaqueMemberPointers;
+  llvm::DenseMap<const Type *, llvm::Type *> RecordsWithOpaqueMemberPointers;
 
   /// Helper for ConvertType.
   llvm::Type *ConvertFunctionTypeInternal(QualType FT);
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -29,16 +29,6 @@
 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()),
@@ -437,14 +427,12 @@
         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.
@@ -784,8 +772,14 @@
   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 I = RecordsWithOpaqueMemberPointers.find(C);
+      if (I == RecordsWithOpaqueMemberPointers.end()) {
+        ResultType = llvm::StructType::create(getLLVMContext());
+        RecordsWithOpaqueMemberPointers.insert({C, ResultType});
+      } else {
+        ResultType = I->second;
+      }
     } else {
       ResultType = getCXXABI().ConvertMemberPointerType(MPTy);
     }
@@ -823,7 +817,7 @@
 
   assert(ResultType && "Didn't convert a type?");
 
-#ifndef NDEBUG
+#ifdef EXPENSIVE_CHECKS
   if (CachedType) {
     assert(CachedType == ResultType &&
            "Cached type doesn't match computed type");

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits