[IRMover] Don't consider opaque types isomorphic to other types (#138241)

The type mapping in IRMover currently has a decent amount of complexity
related to establishing isomorphism between opaque struct types and
non-opaque struct types. I believe that this is both largely useless at
this point (after some recent clarifications, essentially the only place
where opaque types can still appear are external gobals) and has never
been entirely correct in the first place (because it does this in part
purely based on name rather than use, which means that we effectively
end up assigning semantics to the type name, which is illegal).

As such, I'd like to remove this functionality entirely.

Author: nikic

llvm/lib/Linker/IRMover.cpp

@@ -49,21 +49,6 @@ class TypeMapTy : public ValueMapTypeRemapper {
   /// This is a mapping from a source type to a destination type to use.
   DenseMap<Type *, Type *> MappedTypes;
 
-  /// When checking to see if two subgraphs are isomorphic, we speculatively
-  /// add types to MappedTypes, but keep track of them here in case we need to
-  /// roll back.
-  SmallVector<Type *, 16> SpeculativeTypes;
-
-  SmallVector<StructType *, 16> SpeculativeDstOpaqueTypes;
-
-  /// This is a list of non-opaque structs in the source module that are mapped
-  /// to an opaque struct in the destination module.
-  SmallVector<StructType *, 16> SrcDefinitionsToResolve;
-
-  /// This is the set of opaque types in the destination modules who are
-  /// getting a body from the source module.
-  SmallPtrSet<StructType *, 16> DstResolvedOpaqueTypes;
-
 public:
   TypeMapTy(IRMover::IdentifiedStructTypeSet &DstStructTypesSet)
       : DstStructTypesSet(DstStructTypesSet) {}
@@ -73,10 +58,6 @@ class TypeMapTy : public ValueMapTypeRemapper {
   /// equivalent to the specified type in the source module.
   void addTypeMapping(Type *DstTy, Type *SrcTy);
 
-  /// Produce a body for an opaque type in the dest module from a type
-  /// definition in the source module.
-  Error linkDefinedTypeBodies();
-
   /// Return the mapped type to use for the specified input type from the
   /// source module.
   Type *get(Type *SrcTy);
@@ -88,45 +69,19 @@ class TypeMapTy : public ValueMapTypeRemapper {
 private:
   Type *remapType(Type *SrcTy) override { return get(SrcTy); }
 
-  bool areTypesIsomorphic(Type *DstTy, Type *SrcTy);
+  bool recursivelyAddMappingIfTypesAreIsomorphic(Type *DstTy, Type *SrcTy);
 };
 }
 
 void TypeMapTy::addTypeMapping(Type *DstTy, Type *SrcTy) {
-  assert(SpeculativeTypes.empty());
-  assert(SpeculativeDstOpaqueTypes.empty());
-
-  // Check to see if these types are recursively isomorphic and establish a
-  // mapping between them if so.
-  if (!areTypesIsomorphic(DstTy, SrcTy)) {
-    // Oops, they aren't isomorphic.  Just discard this request by rolling out
-    // any speculative mappings we've established.
-    for (Type *Ty : SpeculativeTypes)
-      MappedTypes.erase(Ty);
-
-    SrcDefinitionsToResolve.resize(SrcDefinitionsToResolve.size() -
-                                   SpeculativeDstOpaqueTypes.size());
-    for (StructType *Ty : SpeculativeDstOpaqueTypes)
-      DstResolvedOpaqueTypes.erase(Ty);
-  } else {
-    // SrcTy and DstTy are recursively ismorphic. We clear names of SrcTy
-    // and all its descendants to lower amount of renaming in LLVM context
-    // Renaming occurs because we load all source modules to the same context
-    // and declaration with existing name gets renamed (i.e Foo -> Foo.42).
-    // As a result we may get several different types in the destination
-    // module, which are in fact the same.
-    for (Type *Ty : SpeculativeTypes)
-      if (auto *STy = dyn_cast<StructType>(Ty))
-        if (STy->hasName())
-          STy->setName("");
-  }
-  SpeculativeTypes.clear();
-  SpeculativeDstOpaqueTypes.clear();
+  recursivelyAddMappingIfTypesAreIsomorphic(DstTy, SrcTy);
 }
 
 /// Recursively walk this pair of types, returning true if they are isomorphic,
-/// false if they are not.
-bool TypeMapTy::areTypesIsomorphic(Type *DstTy, Type *SrcTy) {
+/// false if they are not. Types that were determined to be isomorphic are
+/// added to MappedTypes.
+bool TypeMapTy::recursivelyAddMappingIfTypesAreIsomorphic(Type *DstTy,
+                                                          Type *SrcTy) {
   // Two types with differing kinds are clearly not isomorphic.
   if (DstTy->getTypeID() != SrcTy->getTypeID())
     return false;
@@ -145,29 +100,10 @@ bool TypeMapTy::areTypesIsomorphic(Type *DstTy, Type *SrcTy) {
 
   // Okay, we have two types with identical kinds that we haven't seen before.
 
-  // If this is an opaque struct type, special case it.
+  // Always consider opaque struct types non-isomorphic.
   if (StructType *SSTy = dyn_cast<StructType>(SrcTy)) {
-    // Mapping an opaque type to any struct, just keep the dest struct.
-    if (SSTy->isOpaque()) {
-      Entry = DstTy;
-      SpeculativeTypes.push_back(SrcTy);
-      return true;
-    }
-
-    // Mapping a non-opaque source type to an opaque dest.  If this is the first
-    // type that we're mapping onto this destination type then we succeed.  Keep
-    // the dest, but fill it in later. If this is the second (different) type
-    // that we're trying to map onto the same opaque type then we fail.
-    if (cast<StructType>(DstTy)->isOpaque()) {
-      // We can only map one source type onto the opaque destination type.
-      if (!DstResolvedOpaqueTypes.insert(cast<StructType>(DstTy)).second)
-        return false;
-      SrcDefinitionsToResolve.push_back(SSTy);
-      SpeculativeTypes.push_back(SrcTy);
-      SpeculativeDstOpaqueTypes.push_back(cast<StructType>(DstTy));
-      Entry = DstTy;
-      return true;
-    }
+    if (SSTy->isOpaque() || cast<StructType>(DstTy)->isOpaque())
+      return false;
   }
 
   // If the number of subtypes disagree between the two types, then we fail.
@@ -196,38 +132,27 @@ bool TypeMapTy::areTypesIsomorphic(Type *DstTy, Type *SrcTy) {
       return false;
   }
 
-  // Otherwise, we speculate that these two types will line up and recursively
-  // check the subelements.
-  Entry = DstTy;
-  SpeculativeTypes.push_back(SrcTy);
-
+  // Recursively check the subelements.
   for (unsigned I = 0, E = SrcTy->getNumContainedTypes(); I != E; ++I)
-    if (!areTypesIsomorphic(DstTy->getContainedType(I),
-                            SrcTy->getContainedType(I)))
+    if (!recursivelyAddMappingIfTypesAreIsomorphic(DstTy->getContainedType(I),
+                                                   SrcTy->getContainedType(I)))
       return false;
 
   // If everything seems to have lined up, then everything is great.
-  return true;
-}
+  [[maybe_unused]] auto Res = MappedTypes.insert({SrcTy, DstTy});
+  assert(!Res.second && "Recursive type?");
 
-Error TypeMapTy::linkDefinedTypeBodies() {
-  SmallVector<Type *, 16> Elements;
-  for (StructType *SrcSTy : SrcDefinitionsToResolve) {
-    StructType *DstSTy = cast<StructType>(MappedTypes[SrcSTy]);
-    assert(DstSTy->isOpaque());
-
-    // Map the body of the source type over to a new body for the dest type.
-    Elements.resize(SrcSTy->getNumElements());
-    for (unsigned I = 0, E = Elements.size(); I != E; ++I)
-      Elements[I] = get(SrcSTy->getElementType(I));
-
-    if (auto E = DstSTy->setBodyOrError(Elements, SrcSTy->isPacked()))
-      return E;
-    DstStructTypesSet.switchToNonOpaque(DstSTy);
+  if (auto *STy = dyn_cast<StructType>(SrcTy)) {
+    // We clear name of SrcTy to lower amount of renaming in LLVM context.
+    // Renaming occurs because we load all source modules to the same context
+    // and declaration with existing name gets renamed (i.e Foo -> Foo.42).
+    // As a result we may get several different types in the destination
+    // module, which are in fact the same.
+    if (STy->hasName())
+      STy->setName("");
   }
-  SrcDefinitionsToResolve.clear();
-  DstResolvedOpaqueTypes.clear();
-  return Error::success();
+
+  return true;
 }
 
 Type *TypeMapTy::get(Type *Ty) {
@@ -845,10 +770,6 @@ void IRLinker::computeTypeMapping() {
     if (TypeMap.DstStructTypesSet.hasType(DST))
       TypeMap.addTypeMapping(DST, ST);
   }
-
-  // Now that we have discovered all of the type equivalences, get a body for
-  // any 'opaque' types in the dest module that are now resolved.
-  setError(TypeMap.linkDefinedTypeBodies());
 }
 
 static void getArrayElements(const Constant *C,

llvm/test/Linker/opaque.ll

@@ -1,17 +1,18 @@
 ; RUN: llvm-link %p/opaque.ll %p/Inputs/opaque.ll -S -o - | FileCheck %s
 
-; CHECK-DAG: %A =   type {}
+; CHECK-DAG: %A =   type opaque
+; CHECK-DAG: %A.0 = type {}
 ; CHECK-DAG: %B =   type { %C, %C, ptr }
 ; CHECK-DAG: %B.1 = type { %D, %E, ptr }
 ; CHECK-DAG: %C =   type { %A }
 ; CHECK-DAG: %D =   type { %E }
 ; CHECK-DAG: %E =   type opaque
 
 ; CHECK-DAG: @g1 = external global %B
-; CHECK-DAG: @g2 = external global %A
+; CHECK-DAG: @g2 = external global %A.0
 ; CHECK-DAG: @g3 = external global %B.1
 
-; CHECK-DAG: getelementptr %A, ptr null, i32 0
+; CHECK-DAG: getelementptr %A.0, ptr null, i32 0
 
 %A = type opaque
 %B = type { %C, %C, ptr }

llvm/test/Linker/pr22807.ll

@@ -1,6 +1,8 @@
-; RUN: not llvm-link -S -o - %p/pr22807.ll %p/Inputs/pr22807.ll 2>&1 | FileCheck %s
+; RUN: llvm-link -S -o - %p/pr22807.ll %p/Inputs/pr22807.ll 2>&1 | FileCheck %s
 
-; CHECK: error: identified structure type 'struct.A' is recursive
+; CHECK: %struct.B = type { %struct.A }
+; CHECK: %struct.A = type opaque
+; CHECK: @g = external global %struct.B
 
 %struct.B = type { %struct.A }
 %struct.A = type opaque

llvm/test/Linker/type-unique-dst-types.ll

@@ -2,21 +2,17 @@
 ; RUN:           %p/Inputs/type-unique-dst-types2.ll \
 ; RUN:           %p/Inputs/type-unique-dst-types3.ll -S -o %t1.ll
 ; RUN: cat %t1.ll | FileCheck %s
-; RUN: cat %t1.ll | FileCheck --check-prefix=RENAMED %s
 
-; This tests the importance of keeping track of which types are part of the
-; destination module.
-; When the second input is merged in, the context gets an unused A.11. When
-; the third module is then merged, we should pretend it doesn't exist.
+; The types of @g1 and @g3 can be deduplicated, but @g2 should retain its
+; opaque type, even if it has the same name as a type from a different module.
 
 ; CHECK: %A = type { %B }
 ; CHECK-NEXT: %B = type { i8 }
+; CHECK-NEXT: %A.11 = type opaque
 
 ; CHECK: @g3 = external global %A
 ; CHECK: @g1 = external global %A
-; CHECK: @g2 = external global %A
-
-; RENAMED-NOT: A.11
+; CHECK: @g2 = external global %A.11
 
 %A = type { %B }
 %B = type { i8 }