[llvm] [IRMover] Don't consider opaque types isomorphic to other types (PR #138241)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Fri May 30 08:59:24 PDT 2025
https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/138241
>From 96f60b71c1f6fa79093ff0edcdcc8baf5434cc2c Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Fri, 25 Apr 2025 17:15:44 +0200
Subject: [PATCH 1/2] [IRMover] Don't consider opaque types isomorphic to other
types
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.
---
llvm/lib/Linker/IRMover.cpp | 116 ++++------------------
llvm/test/Linker/opaque.ll | 7 +-
llvm/test/Linker/pr22807.ll | 6 +-
llvm/test/Linker/type-unique-dst-types.ll | 12 +--
4 files changed, 30 insertions(+), 111 deletions(-)
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<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);
@@ -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<StructType>(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<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 +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<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 +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 }
>From 1f455e3e6bc8aea6e2ba444845b05d64153aee0b Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Fri, 30 May 2025 17:56:03 +0200
Subject: [PATCH 2/2] Rename method
---
llvm/lib/Linker/IRMover.cpp | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index c07a538561a01..a8b8a6f90893b 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -69,18 +69,18 @@ class TypeMapTy : public ValueMapTypeRemapper {
private:
Type *remapType(Type *SrcTy) override { return get(SrcTy); }
- bool areTypesIsomorphic(Type *DstTy, Type *SrcTy);
+ bool tryToEstablishTypeIsomorphism(Type *DstTy, Type *SrcTy);
};
}
void TypeMapTy::addTypeMapping(Type *DstTy, Type *SrcTy) {
- // areTypesIsomorphic() will also establish the type mapping as a side effect.
- areTypesIsomorphic(DstTy, SrcTy);
+ tryToEstablishTypeIsomorphism(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::tryToEstablishTypeIsomorphism(Type *DstTy, Type *SrcTy) {
// Two types with differing kinds are clearly not isomorphic.
if (DstTy->getTypeID() != SrcTy->getTypeID())
return false;
@@ -133,7 +133,7 @@ bool TypeMapTy::areTypesIsomorphic(Type *DstTy, Type *SrcTy) {
// Recursively check the subelements.
for (unsigned I = 0, E = SrcTy->getNumContainedTypes(); I != E; ++I)
- if (!areTypesIsomorphic(DstTy->getContainedType(I),
+ if (!tryToEstablishTypeIsomorphism(DstTy->getContainedType(I),
SrcTy->getContainedType(I)))
return false;
More information about the llvm-commits
mailing list