IR: getOrInsertODRUniquedType => DICompositeType::getODRType, NFC

Lift the API for debug info ODR type uniquing up a layer.  Instead of
clients managing the map directly on the LLVMContext, add a static
method to DICompositeType called getODRType and handle the map in the
background.  Also adds DICompositeType::getODRTypeIfExists, so far just
for convenience in the unit tests.

This simplifies the logic in LLParser and BitcodeReader.  Because of
argument spam there are actually a few more lines of code now; I'll see
if I come up with a reasonable way to clean that up.

llvm-svn: 266742

include/llvm/IR/DebugInfoMetadata.h

@@ -825,6 +825,23 @@ public:
 
   TempDICompositeType clone() const { return cloneImpl(); }
 
+  /// Get a DICompositeType with the given ODR identifier.
+  ///
+  /// If \a LLVMContext::isODRUniquingDebugTypes(), gets the mapped
+  /// DICompositeType for the given ODR \c Identifier.  If none exists, creates
+  /// a new node.
+  ///
+  /// Else, returns \c nullptr.
+  static DICompositeType *
+  getODRType(LLVMContext &Context, MDString &Identifier, unsigned Tag,
+             MDString *Name, Metadata *File, unsigned Line, Metadata *Scope,
+             Metadata *BaseType, uint64_t SizeInBits, uint64_t AlignInBits,
+             uint64_t OffsetInBits, unsigned Flags, Metadata *Elements,
+             unsigned RuntimeLang, Metadata *VTableHolder,
+             Metadata *TemplateParams);
+  static DICompositeType *getODRTypeIfExists(LLVMContext &Context,
+                                             MDString &Identifier);
+
   DITypeRef getBaseType() const { return DITypeRef(getRawBaseType()); }
   DINodeArray getElements() const {
     return cast_or_null<MDTuple>(getRawElements());

include/llvm/IR/LLVMContext.h

@@ -121,17 +121,6 @@ public:
   void enableDebugTypeODRUniquing();
   void disableDebugTypeODRUniquing();
 
-  /// Get or insert the DICompositeType mapped to the given string.
-  ///
-  /// Returns the address of the current \a DICompositeType pointer mapped to
-  /// \c S, inserting a mapping to \c nullptr if \c S was not previously
-  /// mapped.  This method has no effect (and returns \c nullptr instead of a
-  /// valid address) if \a isODRUniquingDebugTypes() is \c false.
-  ///
-  /// \post If \a isODRUniquingDebugTypes(), \c S will have a (possibly null)
-  /// mapping.  \note The returned address is only valid until the next call.
-  DICompositeType **getOrInsertODRUniquedType(const MDString &S);
-
   typedef void (*InlineAsmDiagHandlerTy)(const SMDiagnostic&, void *Context,
                                          unsigned LocCookie);
 

lib/AsmParser/LLParser.cpp

@@ -3841,11 +3841,13 @@ bool LLParser::ParseDICompositeType(MDNode *&Result, bool IsDistinct) {
 
   // If this isn't a forward declaration and it has a UUID, check for it in the
   // type map in the context.
-  DICompositeType **MappedT = nullptr;
+  if (!(flags.Val & DINode::FlagFwdDecl) && identifier.Val)
-  if (!(flags.Val & DINode::FlagFwdDecl) && identifier.Val &&
+    if (auto *CT = DICompositeType::getODRType(
-      (MappedT = Context.getOrInsertODRUniquedType(*identifier.Val)) &&
+            Context, *identifier.Val, tag.Val, name.Val, file.Val, line.Val,
-      *MappedT) {
+            scope.Val, baseType.Val, size.Val, align.Val, offset.Val, flags.Val,
-    Result = *MappedT;
+            elements.Val, runtimeLang.Val, vtableHolder.Val,
+            templateParams.Val)) {
+      Result = CT;
       return false;
     }
 
@@ -3856,8 +3858,6 @@ bool LLParser::ParseDICompositeType(MDNode *&Result, bool IsDistinct) {
       (Context, tag.Val, name.Val, file.Val, line.Val, scope.Val, baseType.Val,
        size.Val, align.Val, offset.Val, flags.Val, elements.Val,
        runtimeLang.Val, vtableHolder.Val, templateParams.Val, identifier.Val));
-  if (MappedT)
-    *MappedT = cast<DICompositeType>(Result);
   return false;
 }
 

lib/Bitcode/Reader/BitcodeReader.cpp

@@ -2190,25 +2190,36 @@ std::error_code BitcodeReader::parseMetadata(bool ModuleLevel) {
 
       // If we have a UUID and this is not a forward declaration, lookup the
       // mapping.
+      bool IsDistinct = Record[0];
+      unsigned Tag = Record[1];
+      MDString *Name = getMDString(Record[2]);
+      Metadata *File = getMDOrNull(Record[3]);
+      unsigned Line = Record[4];
+      Metadata *Scope = getMDOrNull(Record[5]);
+      Metadata *BaseType = getMDOrNull(Record[6]);
+      uint64_t SizeInBits = Record[7];
+      uint64_t AlignInBits = Record[8];
+      uint64_t OffsetInBits = Record[9];
       unsigned Flags = Record[10];
+      Metadata *Elements = getMDOrNull(Record[11]);
+      unsigned RuntimeLang = Record[12];
+      Metadata *VTableHolder = getMDOrNull(Record[13]);
+      Metadata *TemplateParams = getMDOrNull(Record[14]);
       auto *Identifier = getMDString(Record[15]);
-      DICompositeType **MappedT = nullptr;
+      DICompositeType *CT = nullptr;
       if (!(Flags & DINode::FlagFwdDecl) && Identifier)
-        MappedT = Context.getOrInsertODRUniquedType(*Identifier);
+        CT = DICompositeType::getODRType(
+            Context, *Identifier, Tag, Name, File, Line, Scope, BaseType,
+            SizeInBits, AlignInBits, OffsetInBits, Flags, Elements, RuntimeLang,
+            VTableHolder, TemplateParams);
 
-      // Use the mapped type node, or create a new one if necessary.
+      // Create a node if we didn't get a lazy ODR type.
-      DICompositeType *CT = MappedT ? *MappedT : nullptr;
+      if (!CT)
-      if (!CT) {
+        CT = GET_OR_DISTINCT(DICompositeType, IsDistinct,
-        CT = GET_OR_DISTINCT(
+                             (Context, Tag, Name, File, Line, Scope, BaseType,
-            DICompositeType, Record[0],
+                              SizeInBits, AlignInBits, OffsetInBits, Flags,
-            (Context, Record[1], getMDString(Record[2]), getMDOrNull(Record[3]),
+                              Elements, RuntimeLang, VTableHolder,
-             Record[4], getMDOrNull(Record[5]), getMDOrNull(Record[6]),
+                              TemplateParams, Identifier));
-             Record[7], Record[8], Record[9], Flags, getMDOrNull(Record[11]),
-             Record[12], getMDOrNull(Record[13]), getMDOrNull(Record[14]),
-             Identifier));
-        if (MappedT)
-          *MappedT = CT;
-      }
 
       MetadataList.assignValue(CT, NextMetadataNo++);
       break;

lib/IR/DebugInfoMetadata.cpp

@@ -266,6 +266,32 @@ DICompositeType *DICompositeType::getImpl(
                        Ops);
 }
 
+DICompositeType *DICompositeType::getODRType(
+    LLVMContext &Context, MDString &Identifier, unsigned Tag, MDString *Name,
+    Metadata *File, unsigned Line, Metadata *Scope, Metadata *BaseType,
+    uint64_t SizeInBits, uint64_t AlignInBits, uint64_t OffsetInBits,
+    unsigned Flags, Metadata *Elements, unsigned RuntimeLang,
+    Metadata *VTableHolder, Metadata *TemplateParams) {
+  assert(!Identifier.getString().empty() && "Expected valid identifier");
+  if (!Context.isODRUniquingDebugTypes())
+    return nullptr;
+  auto *&CT = (*Context.pImpl->DITypeMap)[&Identifier];
+  if (!CT)
+    CT = DICompositeType::getDistinct(
+        Context, Tag, Name, File, Line, Scope, BaseType, SizeInBits,
+        AlignInBits, OffsetInBits, Flags, Elements, RuntimeLang, VTableHolder,
+        TemplateParams, &Identifier);
+  return CT;
+}
+
+DICompositeType *DICompositeType::getODRTypeIfExists(LLVMContext &Context,
+                                                     MDString &Identifier) {
+  assert(!Identifier.getString().empty() && "Expected valid identifier");
+  if (!Context.isODRUniquingDebugTypes())
+    return nullptr;
+  return Context.pImpl->DITypeMap->lookup(&Identifier);
+}
+
 DISubroutineType *DISubroutineType::getImpl(LLVMContext &Context,
                                             unsigned Flags, Metadata *TypeArray,
                                             StorageType Storage,

lib/IR/LLVMContext.cpp

@@ -323,12 +323,6 @@ void LLVMContext::enableDebugTypeODRUniquing() {
 
 void LLVMContext::disableDebugTypeODRUniquing() { pImpl->DITypeMap.reset(); }
 
-DICompositeType **LLVMContext::getOrInsertODRUniquedType(const MDString &S) {
-  if (!isODRUniquingDebugTypes())
-    return nullptr;
-  return &(*pImpl->DITypeMap)[&S];
-}
-
 void LLVMContext::setDiscardValueNames(bool Discard) {
   pImpl->DiscardValueNames = Discard;
 }

test/Linker/dicompositetype-unique.ll

@@ -21,11 +21,11 @@
 !named = !{!0, !1}
 
 ; Check both directions.
-; CHECK:        !1 = !DICompositeType(
+; CHECK:        !1 = distinct !DICompositeType(
 ; CHECK-SAME:                         name: "T1"
 ; CHECK-SAME:                         identifier: "T"
 ; CHECK-NOT:       identifier: "T"
-; REVERSE:      !1 = !DICompositeType(
+; REVERSE:      !1 = distinct !DICompositeType(
 ; REVERSE-SAME:                       name: "T2"
 ; REVERSE-SAME:                       identifier: "T"
 ; REVERSE-NOT:     identifier: "T"

unittests/IR/LLVMContextTest.cpp

@@ -25,35 +25,41 @@ TEST(LLVMContextTest, enableDebugTypeODRUniquing) {
 
 TEST(LLVMContextTest, getOrInsertODRUniquedType) {
   LLVMContext Context;
-  const MDString &S = *MDString::get(Context, "string");
+  MDString &UUID = *MDString::get(Context, "string");
 
   // Without a type map, this should return null.
-  EXPECT_FALSE(Context.getOrInsertODRUniquedType(S));
+  EXPECT_FALSE(DICompositeType::getODRType(
+      Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr, 0, nullptr,
+      nullptr, 0, 0, 0, 0, nullptr, 0, nullptr, nullptr));
 
-  // Get the mapping.
+  // Enable the mapping.  There still shouldn't be a type.
   Context.enableDebugTypeODRUniquing();
-  DICompositeType **Mapping = Context.getOrInsertODRUniquedType(S);
+  EXPECT_FALSE(DICompositeType::getODRTypeIfExists(Context, UUID));
-  ASSERT_TRUE(Mapping);
 
-  // Create some type and add it to the mapping.
+  // Create some ODR-uniqued type.
-  auto &CT = *DICompositeType::get(Context, dwarf::DW_TAG_class_type, "name",
+  auto &CT = *DICompositeType::getODRType(
-                                   nullptr, 0, nullptr, nullptr, 0, 0, 0, 0,
+      Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr, 0, nullptr,
-                                   nullptr, 0, nullptr, nullptr, S.getString());
+      nullptr, 0, 0, 0, 0, nullptr, 0, nullptr, nullptr);
-  ASSERT_EQ(S.getString(), CT.getIdentifier());
+  EXPECT_EQ(UUID.getString(), CT.getIdentifier());
-  *Mapping = &CT;
 
-  // Check that we get it back.
+  // Check that we get it back, even if we change a field.
-  Mapping = Context.getOrInsertODRUniquedType(S);
+  EXPECT_EQ(&CT, DICompositeType::getODRTypeIfExists(Context, UUID));
-  ASSERT_TRUE(Mapping);
+  EXPECT_EQ(
-  EXPECT_EQ(&CT, *Mapping);
+      &CT, DICompositeType::getODRType(Context, UUID, dwarf::DW_TAG_class_type,
+                                       nullptr, nullptr, 0, nullptr, nullptr, 0,
+                                       0, 0, 0, nullptr, 0, nullptr, nullptr));
+  EXPECT_EQ(&CT, DICompositeType::getODRType(
+                     Context, UUID, dwarf::DW_TAG_class_type,
+                     MDString::get(Context, "name"), nullptr, 0, nullptr,
+                     nullptr, 0, 0, 0, 0, nullptr, 0, nullptr, nullptr));
 
   // Check that it's discarded with the type map.
   Context.disableDebugTypeODRUniquing();
-  EXPECT_FALSE(Context.getOrInsertODRUniquedType(S));
+  EXPECT_FALSE(DICompositeType::getODRTypeIfExists(Context, UUID));
 
   // And it shouldn't magically reappear...
   Context.enableDebugTypeODRUniquing();
-  EXPECT_FALSE(*Context.getOrInsertODRUniquedType(S));
+  EXPECT_FALSE(DICompositeType::getODRTypeIfExists(Context, UUID));
 }
 
 } // end namespace