diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp index 3df3ee84ce214..c07a538561a01 100644 --- a/llvm/lib/Linker/IRMover.cpp +++ b/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 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 SpeculativeTypes; - - SmallVector 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 SrcDefinitionsToResolve; - - /// This is the set of opaque types in the destination modules who are - /// getting a body from the source module. - SmallPtrSet 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); @@ -93,35 +74,8 @@ class TypeMapTy : public ValueMapTypeRemapper { } 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(Ty)) - if (STy->hasName()) - STy->setName(""); - } - SpeculativeTypes.clear(); - SpeculativeDstOpaqueTypes.clear(); + // areTypesIsomorphic() will also establish the type mapping as a side effect. + areTypesIsomorphic(DstTy, SrcTy); } /// Recursively walk this pair of types, returning true if they are isomorphic, @@ -145,29 +99,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(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(DstTy)->isOpaque()) { - // We can only map one source type onto the opaque destination type. - if (!DstResolvedOpaqueTypes.insert(cast(DstTy)).second) - return false; - SrcDefinitionsToResolve.push_back(SSTy); - SpeculativeTypes.push_back(SrcTy); - SpeculativeDstOpaqueTypes.push_back(cast(DstTy)); - Entry = DstTy; - return true; - } + if (SSTy->isOpaque() || cast(DstTy)->isOpaque()) + return false; } // If the number of subtypes disagree between the two types, then we fail. @@ -196,38 +131,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))) 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 Elements; - for (StructType *SrcSTy : SrcDefinitionsToResolve) { - StructType *DstSTy = cast(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(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 +769,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, diff --git a/llvm/test/Linker/opaque.ll b/llvm/test/Linker/opaque.ll index 7d2c49af7797f..2b66e3d8c7dd1 100644 --- a/llvm/test/Linker/opaque.ll +++ b/llvm/test/Linker/opaque.ll @@ -1,6 +1,7 @@ ; 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 } @@ -8,10 +9,10 @@ ; 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 } diff --git a/llvm/test/Linker/pr22807.ll b/llvm/test/Linker/pr22807.ll index 24bcf1732112b..69944b84cd50a 100644 --- a/llvm/test/Linker/pr22807.ll +++ b/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 diff --git a/llvm/test/Linker/type-unique-dst-types.ll b/llvm/test/Linker/type-unique-dst-types.ll index c5fcbd3f51fdb..54f0ef40333b2 100644 --- a/llvm/test/Linker/type-unique-dst-types.ll +++ b/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 }