ValueMapper: Delete unused initializers of replaced appending globals. #167629
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.6-beta.1
llvmbot
added
LTO
Member
|
@llvm/pr-subscribers-lto Author: Peter Collingbourne (pcc) ChangesA full LTO link time performance and memory regression was introduced The repro-cfi-64 reproducer from #167037 before and after this change: Fixes #167037. Full diff: https://github.com/llvm/llvm-project/pull/167629.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/ValueMapper.h b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
index 17b5d4b891230..28c4ae840b29f 100644
--- a/llvm/include/llvm/Transforms/Utils/ValueMapper.h
+++ b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
@@ -204,7 +204,7 @@ class ValueMapper {
LLVM_ABI void scheduleMapGlobalInitializer(GlobalVariable &GV, Constant &Init,
unsigned MappingContextID = 0);
LLVM_ABI void scheduleMapAppendingVariable(GlobalVariable &GV,
- Constant *InitPrefix,
+ GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MappingContextID = 0);
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index f78d9b016d8c9..f215f39f41bfb 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -882,10 +882,7 @@ IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
NG->copyAttributesFrom(SrcGV);
forceRenaming(NG, SrcGV->getName());
- Mapper.scheduleMapAppendingVariable(
- *NG,
- (DstGV && !DstGV->isDeclaration()) ? DstGV->getInitializer() : nullptr,
- IsOldStructor, SrcElements);
+ Mapper.scheduleMapAppendingVariable(*NG, DstGV, IsOldStructor, SrcElements);
// Replace any uses of the two global variables with uses of the new
// global.
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 8d8a60b6918fe..9021d8b289baf 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -77,7 +77,7 @@ struct WorklistEntry {
};
struct AppendingGVTy {
GlobalVariable *GV;
- Constant *InitPrefix;
+ GlobalVariable *OldGV;
};
struct AliasOrIFuncTy {
GlobalValue *GV;
@@ -162,7 +162,7 @@ class Mapper {
void scheduleMapGlobalInitializer(GlobalVariable &GV, Constant &Init,
unsigned MCID);
- void scheduleMapAppendingVariable(GlobalVariable &GV, Constant *InitPrefix,
+ void scheduleMapAppendingVariable(GlobalVariable &GV, GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MCID);
@@ -173,7 +173,7 @@ class Mapper {
void flush();
private:
- void mapAppendingVariable(GlobalVariable &GV, Constant *InitPrefix,
+ void mapAppendingVariable(GlobalVariable &GV, GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers);
@@ -944,7 +944,7 @@ void Mapper::flush() {
drop_begin(AppendingInits, PrefixSize));
AppendingInits.resize(PrefixSize);
mapAppendingVariable(*E.Data.AppendingGV.GV,
- E.Data.AppendingGV.InitPrefix,
+ E.Data.AppendingGV.OldGV,
E.AppendingGVIsOldCtorDtor, ArrayRef(NewInits));
break;
}
@@ -1094,15 +1094,21 @@ void Mapper::remapFunction(Function &F) {
}
}
-void Mapper::mapAppendingVariable(GlobalVariable &GV, Constant *InitPrefix,
+void Mapper::mapAppendingVariable(GlobalVariable &GV, GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers) {
+ Constant *InitPrefix =
+ (OldGV && !OldGV->isDeclaration()) ? OldGV->getInitializer() : nullptr;
+
SmallVector<Constant *, 16> Elements;
if (InitPrefix) {
unsigned NumElements =
cast<ArrayType>(InitPrefix->getType())->getNumElements();
for (unsigned I = 0; I != NumElements; ++I)
Elements.push_back(InitPrefix->getAggregateElement(I));
+ OldGV->setInitializer(nullptr);
+ if (InitPrefix->hasUseList() && InitPrefix->use_empty())
+ InitPrefix->destroyConstant();
}
PointerType *VoidPtrTy;
@@ -1148,7 +1154,7 @@ void Mapper::scheduleMapGlobalInitializer(GlobalVariable &GV, Constant &Init,
}
void Mapper::scheduleMapAppendingVariable(GlobalVariable &GV,
- Constant *InitPrefix,
+ GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MCID) {
@@ -1159,7 +1165,7 @@ void Mapper::scheduleMapAppendingVariable(GlobalVariable &GV,
WE.Kind = WorklistEntry::MapAppendingVar;
WE.MCID = MCID;
WE.Data.AppendingGV.GV = &GV;
- WE.Data.AppendingGV.InitPrefix = InitPrefix;
+ WE.Data.AppendingGV.OldGV = OldGV;
WE.AppendingGVIsOldCtorDtor = IsOldCtorDtor;
WE.AppendingGVNumNewMembers = NewMembers.size();
Worklist.push_back(WE);
@@ -1282,12 +1288,12 @@ void ValueMapper::scheduleMapGlobalInitializer(GlobalVariable &GV,
}
void ValueMapper::scheduleMapAppendingVariable(GlobalVariable &GV,
- Constant *InitPrefix,
+ GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MCID) {
getAsMapper(pImpl)->scheduleMapAppendingVariable(
- GV, InitPrefix, IsOldCtorDtor, NewMembers, MCID);
+ GV, OldGV, IsOldCtorDtor, NewMembers, MCID);
}
void ValueMapper::scheduleMapGlobalAlias(GlobalAlias &GA, Constant &Aliasee,
|
Member
|
@llvm/pr-subscribers-llvm-transforms Author: Peter Collingbourne (pcc) ChangesA full LTO link time performance and memory regression was introduced The repro-cfi-64 reproducer from #167037 before and after this change: Fixes #167037. Full diff: https://github.com/llvm/llvm-project/pull/167629.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/ValueMapper.h b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
index 17b5d4b891230..28c4ae840b29f 100644
--- a/llvm/include/llvm/Transforms/Utils/ValueMapper.h
+++ b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
@@ -204,7 +204,7 @@ class ValueMapper {
LLVM_ABI void scheduleMapGlobalInitializer(GlobalVariable &GV, Constant &Init,
unsigned MappingContextID = 0);
LLVM_ABI void scheduleMapAppendingVariable(GlobalVariable &GV,
- Constant *InitPrefix,
+ GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MappingContextID = 0);
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index f78d9b016d8c9..f215f39f41bfb 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -882,10 +882,7 @@ IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
NG->copyAttributesFrom(SrcGV);
forceRenaming(NG, SrcGV->getName());
- Mapper.scheduleMapAppendingVariable(
- *NG,
- (DstGV && !DstGV->isDeclaration()) ? DstGV->getInitializer() : nullptr,
- IsOldStructor, SrcElements);
+ Mapper.scheduleMapAppendingVariable(*NG, DstGV, IsOldStructor, SrcElements);
// Replace any uses of the two global variables with uses of the new
// global.
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 8d8a60b6918fe..9021d8b289baf 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -77,7 +77,7 @@ struct WorklistEntry {
};
struct AppendingGVTy {
GlobalVariable *GV;
- Constant *InitPrefix;
+ GlobalVariable *OldGV;
};
struct AliasOrIFuncTy {
GlobalValue *GV;
@@ -162,7 +162,7 @@ class Mapper {
void scheduleMapGlobalInitializer(GlobalVariable &GV, Constant &Init,
unsigned MCID);
- void scheduleMapAppendingVariable(GlobalVariable &GV, Constant *InitPrefix,
+ void scheduleMapAppendingVariable(GlobalVariable &GV, GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MCID);
@@ -173,7 +173,7 @@ class Mapper {
void flush();
private:
- void mapAppendingVariable(GlobalVariable &GV, Constant *InitPrefix,
+ void mapAppendingVariable(GlobalVariable &GV, GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers);
@@ -944,7 +944,7 @@ void Mapper::flush() {
drop_begin(AppendingInits, PrefixSize));
AppendingInits.resize(PrefixSize);
mapAppendingVariable(*E.Data.AppendingGV.GV,
- E.Data.AppendingGV.InitPrefix,
+ E.Data.AppendingGV.OldGV,
E.AppendingGVIsOldCtorDtor, ArrayRef(NewInits));
break;
}
@@ -1094,15 +1094,21 @@ void Mapper::remapFunction(Function &F) {
}
}
-void Mapper::mapAppendingVariable(GlobalVariable &GV, Constant *InitPrefix,
+void Mapper::mapAppendingVariable(GlobalVariable &GV, GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers) {
+ Constant *InitPrefix =
+ (OldGV && !OldGV->isDeclaration()) ? OldGV->getInitializer() : nullptr;
+
SmallVector<Constant *, 16> Elements;
if (InitPrefix) {
unsigned NumElements =
cast<ArrayType>(InitPrefix->getType())->getNumElements();
for (unsigned I = 0; I != NumElements; ++I)
Elements.push_back(InitPrefix->getAggregateElement(I));
+ OldGV->setInitializer(nullptr);
+ if (InitPrefix->hasUseList() && InitPrefix->use_empty())
+ InitPrefix->destroyConstant();
}
PointerType *VoidPtrTy;
@@ -1148,7 +1154,7 @@ void Mapper::scheduleMapGlobalInitializer(GlobalVariable &GV, Constant &Init,
}
void Mapper::scheduleMapAppendingVariable(GlobalVariable &GV,
- Constant *InitPrefix,
+ GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MCID) {
@@ -1159,7 +1165,7 @@ void Mapper::scheduleMapAppendingVariable(GlobalVariable &GV,
WE.Kind = WorklistEntry::MapAppendingVar;
WE.MCID = MCID;
WE.Data.AppendingGV.GV = &GV;
- WE.Data.AppendingGV.InitPrefix = InitPrefix;
+ WE.Data.AppendingGV.OldGV = OldGV;
WE.AppendingGVIsOldCtorDtor = IsOldCtorDtor;
WE.AppendingGVNumNewMembers = NewMembers.size();
Worklist.push_back(WE);
@@ -1282,12 +1288,12 @@ void ValueMapper::scheduleMapGlobalInitializer(GlobalVariable &GV,
}
void ValueMapper::scheduleMapAppendingVariable(GlobalVariable &GV,
- Constant *InitPrefix,
+ GlobalVariable *OldGV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MCID) {
getAsMapper(pImpl)->scheduleMapAppendingVariable(
- GV, InitPrefix, IsOldCtorDtor, NewMembers, MCID);
+ GV, OldGV, IsOldCtorDtor, NewMembers, MCID);
}
void ValueMapper::scheduleMapGlobalAlias(GlobalAlias &GA, Constant &Aliasee,
|
Contributor
Author
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp -- llvm/include/llvm/Transforms/Utils/ValueMapper.h llvm/lib/Linker/IRMover.cpp llvm/lib/Transforms/Utils/ValueMapper.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 9021d8b28..e7af826e6 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -943,8 +943,7 @@ void Mapper::flush() {
SmallVector<Constant *, 8> NewInits(
drop_begin(AppendingInits, PrefixSize));
AppendingInits.resize(PrefixSize);
- mapAppendingVariable(*E.Data.AppendingGV.GV,
- E.Data.AppendingGV.OldGV,
+ mapAppendingVariable(*E.Data.AppendingGV.GV, E.Data.AppendingGV.OldGV,
E.AppendingGVIsOldCtorDtor, ArrayRef(NewInits));
break;
}
@@ -1292,8 +1291,8 @@ void ValueMapper::scheduleMapAppendingVariable(GlobalVariable &GV,
bool IsOldCtorDtor,
ArrayRef<Constant *> NewMembers,
unsigned MCID) {
- getAsMapper(pImpl)->scheduleMapAppendingVariable(
- GV, OldGV, IsOldCtorDtor, NewMembers, MCID);
+ getAsMapper(pImpl)->scheduleMapAppendingVariable(GV, OldGV, IsOldCtorDtor,
+ NewMembers, MCID);
}
void ValueMapper::scheduleMapGlobalAlias(GlobalAlias &GA, Constant &Aliasee,
|
teresajohnson
approved these changes
Nov 12, 2025
pcc
deleted the
users/pcc/spr/valuemapper-delete-unused-initializers-of-replaced-appending-globals
branch
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Nov 12, 2025
…ing globals.
A full LTO link time performance and memory regression was introduced
by #137081 in cases where the modules contain large quantities of llvm.used
globals. This was unnoticed because it was not expected that this would
be a typical case, but this is exactly what coverage collection does,
and when this feature is enabled together with full LTO we end up with
quadratic memory consumption (from the unused constants) and quadratic
complexity in the function Verifier::visitGlobalValue (which visits all
the unused constants in the use list of each global value). This is a
targeted fix that avoids reintroducing the quadratic complexity from
before #137081, by having ValueMapper delete the old initializer of an
appending global if it is unused, instead of visiting every global in
the context after every link.
The repro-cfi-64 reproducer from #167037 before and after this change:
```
Elapsed time Max RSS (KB)
Before 12:05.11 52537184
After 3:27.68 7520696
```
Fixes #167037.
Reviewers: nikic, teresajohnson
Reviewed By: teresajohnson
Pull Request: llvm/llvm-project#167629
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A full LTO link time performance and memory regression was introduced
by #137081 in cases where the modules contain large quantities of llvm.used
globals. This was unnoticed because it was not expected that this would
be a typical case, but this is exactly what coverage collection does,
and when this feature is enabled together with full LTO we end up with
quadratic memory consumption (from the unused constants) and quadratic
complexity in the function Verifier::visitGlobalValue (which visits all
the unused constants in the use list of each global value). This is a
targeted fix that avoids reintroducing the quadratic complexity from
before #137081, by having ValueMapper delete the old initializer of an
appending global if it is unused, instead of visiting every global in
the context after every link.
The repro-cfi-64 reproducer from #167037 before and after this change:
Fixes #167037.