[llvm] [NFC][Cloning] Clean up comments in CloneFunctionInto (PR #129153)
Artem Pianykh via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 14 05:16:33 PDT 2025
https://github.com/artempyanykh updated https://github.com/llvm/llvm-project/pull/129153
>From e874152180d03cc8b69ac1d7467b053d5b59a746 Mon Sep 17 00:00:00 2001
From: Artem Pianykh <arr at fb.com>
Date: Tue, 25 Feb 2025 12:42:14 -0800
Subject: [PATCH 1/5] [NFC][Coro] Use CloneFunctionInto for coroutine cloning
instead of CloneFunction<Part>
Summary:
CloneFunctionInto now is fast on its own and we don't need to use
CloneFunctionAttributes/Metadata/Body separately.
CommonDebugInfo in CoroClone is now unused and is cleaned up separately
in the next diff in the stack.
Test Plan:
ninja check-all
stack-info: PR: https://github.com/llvm/llvm-project/pull/129149, branch: users/artempyanykh/fast-coro-upstream-part2-take2/7
---
llvm/lib/Transforms/Coroutines/CoroSplit.cpp | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index b2c4e64319725..fabbf5f020a74 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -921,14 +921,8 @@ void coro::BaseCloner::create() {
auto savedLinkage = NewF->getLinkage();
NewF->setLinkage(llvm::GlobalValue::ExternalLinkage);
- MetadataPredicate IdentityMD = [&](const Metadata *MD) {
- return CommonDebugInfo.contains(MD);
- };
- CloneFunctionAttributesInto(NewF, &OrigF, VMap, false);
- CloneFunctionMetadataInto(*NewF, OrigF, VMap, RF_None, nullptr, nullptr,
- &IdentityMD);
- CloneFunctionBodyInto(*NewF, OrigF, VMap, RF_None, Returns, "", nullptr,
- nullptr, nullptr, &IdentityMD);
+ CloneFunctionInto(NewF, &OrigF, VMap,
+ CloneFunctionChangeType::LocalChangesOnly, Returns);
auto &Context = NewF->getContext();
>From 2221cebe6ce50cd816a1580f5f9ee9b0e494389d Mon Sep 17 00:00:00 2001
From: Artem Pianykh <arr at fb.com>
Date: Tue, 25 Feb 2025 12:47:10 -0800
Subject: [PATCH 2/5] [NFC][Coro] Remove now unused CommonDebugInfo in
CoroSplit
Summary:
This cleans up the now unnecessary debug info collection in CoroSplit.
This makes CoroSplit pass almost as fast with -g2 as it is with -g1 on
the sample cpp file used with other parts of this stack:
| | Baseline | IdentityMD set | Prebuilt CommonDI | MetadataPred (cur) |
|-----------------|----------|----------------|-------------------|--------------------|
| CoroSplitPass | 306ms | 221ms | 68ms | 3.8ms |
| CoroCloner | 101ms | 72ms | 0.5ms | 0.5ms |
| CollectCommonDI | - | - | 63ms | - |
| Speed up | 1x | 1.4x | 4.5x | 80x |
Test Plan:
ninja check-all
stack-info: PR: https://github.com/llvm/llvm-project/pull/129150, branch: users/artempyanykh/fast-coro-upstream-part2-take2/8
---
llvm/lib/Transforms/Coroutines/CoroCloner.h | 31 ++++++----------
llvm/lib/Transforms/Coroutines/CoroSplit.cpp | 37 +++-----------------
2 files changed, 16 insertions(+), 52 deletions(-)
diff --git a/llvm/lib/Transforms/Coroutines/CoroCloner.h b/llvm/lib/Transforms/Coroutines/CoroCloner.h
index b817e55cad9fc..d1887980fb3bc 100644
--- a/llvm/lib/Transforms/Coroutines/CoroCloner.h
+++ b/llvm/lib/Transforms/Coroutines/CoroCloner.h
@@ -48,9 +48,6 @@ class BaseCloner {
CloneKind FKind;
IRBuilder<> Builder;
TargetTransformInfo &TTI;
- // Common module-level metadata that's shared between all coroutine clones and
- // doesn't need to be cloned itself.
- const MetadataSetTy &CommonDebugInfo;
ValueToValueMapTy VMap;
Function *NewF = nullptr;
@@ -63,12 +60,12 @@ class BaseCloner {
/// Create a cloner for a continuation lowering.
BaseCloner(Function &OrigF, const Twine &Suffix, coro::Shape &Shape,
Function *NewF, AnyCoroSuspendInst *ActiveSuspend,
- TargetTransformInfo &TTI, const MetadataSetTy &CommonDebugInfo)
+ TargetTransformInfo &TTI)
: OrigF(OrigF), Suffix(Suffix), Shape(Shape),
FKind(Shape.ABI == ABI::Async ? CloneKind::Async
: CloneKind::Continuation),
- Builder(OrigF.getContext()), TTI(TTI), CommonDebugInfo(CommonDebugInfo),
- NewF(NewF), ActiveSuspend(ActiveSuspend) {
+ Builder(OrigF.getContext()), TTI(TTI), NewF(NewF),
+ ActiveSuspend(ActiveSuspend) {
assert(Shape.ABI == ABI::Retcon || Shape.ABI == ABI::RetconOnce ||
Shape.ABI == ABI::Async);
assert(NewF && "need existing function for continuation");
@@ -77,11 +74,9 @@ class BaseCloner {
public:
BaseCloner(Function &OrigF, const Twine &Suffix, coro::Shape &Shape,
- CloneKind FKind, TargetTransformInfo &TTI,
- const MetadataSetTy &CommonDebugInfo)
+ CloneKind FKind, TargetTransformInfo &TTI)
: OrigF(OrigF), Suffix(Suffix), Shape(Shape), FKind(FKind),
- Builder(OrigF.getContext()), TTI(TTI),
- CommonDebugInfo(CommonDebugInfo) {}
+ Builder(OrigF.getContext()), TTI(TTI) {}
virtual ~BaseCloner() {}
@@ -89,14 +84,12 @@ class BaseCloner {
static Function *createClone(Function &OrigF, const Twine &Suffix,
coro::Shape &Shape, Function *NewF,
AnyCoroSuspendInst *ActiveSuspend,
- TargetTransformInfo &TTI,
- const MetadataSetTy &CommonDebugInfo) {
+ TargetTransformInfo &TTI) {
assert(Shape.ABI == ABI::Retcon || Shape.ABI == ABI::RetconOnce ||
Shape.ABI == ABI::Async);
TimeTraceScope FunctionScope("BaseCloner");
- BaseCloner Cloner(OrigF, Suffix, Shape, NewF, ActiveSuspend, TTI,
- CommonDebugInfo);
+ BaseCloner Cloner(OrigF, Suffix, Shape, NewF, ActiveSuspend, TTI);
Cloner.create();
return Cloner.getFunction();
}
@@ -136,9 +129,8 @@ class SwitchCloner : public BaseCloner {
protected:
/// Create a cloner for a switch lowering.
SwitchCloner(Function &OrigF, const Twine &Suffix, coro::Shape &Shape,
- CloneKind FKind, TargetTransformInfo &TTI,
- const MetadataSetTy &CommonDebugInfo)
- : BaseCloner(OrigF, Suffix, Shape, FKind, TTI, CommonDebugInfo) {}
+ CloneKind FKind, TargetTransformInfo &TTI)
+ : BaseCloner(OrigF, Suffix, Shape, FKind, TTI) {}
void create() override;
@@ -146,12 +138,11 @@ class SwitchCloner : public BaseCloner {
/// Create a clone for a switch lowering.
static Function *createClone(Function &OrigF, const Twine &Suffix,
coro::Shape &Shape, CloneKind FKind,
- TargetTransformInfo &TTI,
- const MetadataSetTy &CommonDebugInfo) {
+ TargetTransformInfo &TTI) {
assert(Shape.ABI == ABI::Switch);
TimeTraceScope FunctionScope("SwitchCloner");
- SwitchCloner Cloner(OrigF, Suffix, Shape, FKind, TTI, CommonDebugInfo);
+ SwitchCloner Cloner(OrigF, Suffix, Shape, FKind, TTI);
Cloner.create();
return Cloner.getFunction();
}
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index fabbf5f020a74..f9a6c70fedc2d 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -78,24 +78,6 @@ using namespace llvm;
#define DEBUG_TYPE "coro-split"
-namespace {
-/// Collect (a known) subset of global debug info metadata potentially used by
-/// the function \p F.
-///
-/// This metadata set can be used to avoid cloning debug info not owned by \p F
-/// and is shared among all potential clones \p F.
-MetadataSetTy collectCommonDebugInfo(Function &F) {
- TimeTraceScope FunctionScope("CollectCommonDebugInfo");
-
- DebugInfoFinder DIFinder;
- DISubprogram *SPClonedWithinModule = CollectDebugInfoForCloning(
- F, CloneFunctionChangeType::LocalChangesOnly, DIFinder);
-
- return FindDebugInfoToIdentityMap(CloneFunctionChangeType::LocalChangesOnly,
- DIFinder, SPClonedWithinModule);
-}
-} // end anonymous namespace
-
// FIXME:
// Lower the intrinisc in CoroEarly phase if coroutine frame doesn't escape
// and it is known that other transformations, for example, sanitizers
@@ -1406,21 +1388,16 @@ struct SwitchCoroutineSplitter {
TargetTransformInfo &TTI) {
assert(Shape.ABI == coro::ABI::Switch);
- MetadataSetTy CommonDebugInfo{collectCommonDebugInfo(F)};
-
// Create a resume clone by cloning the body of the original function,
// setting new entry block and replacing coro.suspend an appropriate value
// to force resume or cleanup pass for every suspend point.
createResumeEntryBlock(F, Shape);
auto *ResumeClone = coro::SwitchCloner::createClone(
- F, ".resume", Shape, coro::CloneKind::SwitchResume, TTI,
- CommonDebugInfo);
+ F, ".resume", Shape, coro::CloneKind::SwitchResume, TTI);
auto *DestroyClone = coro::SwitchCloner::createClone(
- F, ".destroy", Shape, coro::CloneKind::SwitchUnwind, TTI,
- CommonDebugInfo);
+ F, ".destroy", Shape, coro::CloneKind::SwitchUnwind, TTI);
auto *CleanupClone = coro::SwitchCloner::createClone(
- F, ".cleanup", Shape, coro::CloneKind::SwitchCleanup, TTI,
- CommonDebugInfo);
+ F, ".cleanup", Shape, coro::CloneKind::SwitchCleanup, TTI);
postSplitCleanup(*ResumeClone);
postSplitCleanup(*DestroyClone);
@@ -1806,14 +1783,12 @@ void coro::AsyncABI::splitCoroutine(Function &F, coro::Shape &Shape,
assert(Clones.size() == Shape.CoroSuspends.size());
- MetadataSetTy CommonDebugInfo{collectCommonDebugInfo(F)};
-
for (auto [Idx, CS] : llvm::enumerate(Shape.CoroSuspends)) {
auto *Suspend = CS;
auto *Clone = Clones[Idx];
coro::BaseCloner::createClone(F, "resume." + Twine(Idx), Shape, Clone,
- Suspend, TTI, CommonDebugInfo);
+ Suspend, TTI);
}
}
@@ -1940,14 +1915,12 @@ void coro::AnyRetconABI::splitCoroutine(Function &F, coro::Shape &Shape,
assert(Clones.size() == Shape.CoroSuspends.size());
- MetadataSetTy CommonDebugInfo{collectCommonDebugInfo(F)};
-
for (auto [Idx, CS] : llvm::enumerate(Shape.CoroSuspends)) {
auto Suspend = CS;
auto Clone = Clones[Idx];
coro::BaseCloner::createClone(F, "resume." + Twine(Idx), Shape, Clone,
- Suspend, TTI, CommonDebugInfo);
+ Suspend, TTI);
}
}
>From 18be509bc78bd6b4c0692acd0b5c02986e869c08 Mon Sep 17 00:00:00 2001
From: Artem Pianykh <arr at fb.com>
Date: Tue, 25 Feb 2025 13:00:47 -0800
Subject: [PATCH 3/5] [NFC][Cloning] Remove now unused
FindDebugInfoToIdentityMap
Summary:
This function is no longer needed.
Test Plan:
ninja check-llvm-unit
stack-info: PR: https://github.com/llvm/llvm-project/pull/129151, branch: users/artempyanykh/fast-coro-upstream-part2-take2/9
---
llvm/include/llvm/Transforms/Utils/Cloning.h | 19 -----------
llvm/lib/Transforms/Utils/CloneFunction.cpp | 34 --------------------
2 files changed, 53 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/Cloning.h b/llvm/include/llvm/Transforms/Utils/Cloning.h
index 2252dda0b9aad..ae00c16e7eada 100644
--- a/llvm/include/llvm/Transforms/Utils/Cloning.h
+++ b/llvm/include/llvm/Transforms/Utils/Cloning.h
@@ -244,25 +244,6 @@ DISubprogram *CollectDebugInfoForCloning(const Function &F,
CloneFunctionChangeType Changes,
DebugInfoFinder &DIFinder);
-/// Based on \p Changes and \p DIFinder return debug info that needs to be
-/// identity mapped during Metadata cloning.
-///
-/// NOTE: Such \a MetadataSetTy can be used by \a CloneFunction* to directly
-/// specify metadata that should be identity mapped (and hence not cloned). The
-/// metadata will be identity mapped in \a ValueToValueMapTy on first use. There
-/// are several reasons for doing it this way rather than eagerly identity
-/// mapping metadata nodes in a \a ValueMap:
-/// 1. Mapping metadata is not cheap, particularly because of tracking.
-/// 2. When cloning a Function we identity map lots of global module-level
-/// metadata to avoid cloning it, while only a fraction of it is actually
-/// used by the function. Mapping on first use is a lot faster for modules
-/// with meaningful amount of debug info.
-/// 3. Eagerly identity mapping metadata makes it harder to cache module-level
-/// data (e.g. a set of metadata nodes in a \a DICompileUnit).
-MetadataSetTy FindDebugInfoToIdentityMap(CloneFunctionChangeType Changes,
- DebugInfoFinder &DIFinder,
- DISubprogram *SPClonedWithinModule);
-
/// This class captures the data input to the InlineFunction call, and records
/// the auxiliary results produced by it.
class InlineFunctionInfo {
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index bdabe0e562fc9..249bef4696b8a 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -189,40 +189,6 @@ DISubprogram *llvm::CollectDebugInfoForCloning(const Function &F,
return SPClonedWithinModule;
}
-MetadataSetTy
-llvm::FindDebugInfoToIdentityMap(CloneFunctionChangeType Changes,
- DebugInfoFinder &DIFinder,
- DISubprogram *SPClonedWithinModule) {
- if (Changes >= CloneFunctionChangeType::DifferentModule)
- return {};
-
- if (DIFinder.subprogram_count() == 0)
- assert(!SPClonedWithinModule &&
- "Subprogram should be in DIFinder->subprogram_count()...");
-
- MetadataSetTy MD;
-
- // Avoid cloning types, compile units, and (other) subprograms.
- for (DISubprogram *ISP : DIFinder.subprograms())
- if (ISP != SPClonedWithinModule)
- MD.insert(ISP);
-
- // If a subprogram isn't going to be cloned skip its lexical blocks as well.
- for (DIScope *S : DIFinder.scopes()) {
- auto *LScope = dyn_cast<DILocalScope>(S);
- if (LScope && LScope->getSubprogram() != SPClonedWithinModule)
- MD.insert(S);
- }
-
- for (DICompileUnit *CU : DIFinder.compile_units())
- MD.insert(CU);
-
- for (DIType *Type : DIFinder.types())
- MD.insert(Type);
-
- return MD;
-}
-
void llvm::CloneFunctionMetadataInto(Function &NewFunc, const Function &OldFunc,
ValueToValueMapTy &VMap,
RemapFlags RemapFlag,
>From e2aef4256aeef2e09b931e7b01c801db709a779b Mon Sep 17 00:00:00 2001
From: Artem Pianykh <arr at fb.com>
Date: Tue, 25 Feb 2025 13:02:37 -0800
Subject: [PATCH 4/5] [NFC][Cloning] Remove now unused
CollectDebugInfoForCloning
Summary:
This function is no longer used, let's remove it from the header and
impl.
Test Plan:
ninja check-llvm-unit
stack-info: PR: https://github.com/llvm/llvm-project/pull/129152, branch: users/artempyanykh/fast-coro-upstream-part2-take2/10
---
llvm/include/llvm/Transforms/Utils/Cloning.h | 14 -------------
llvm/lib/Transforms/Utils/CloneFunction.cpp | 21 --------------------
2 files changed, 35 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/Cloning.h b/llvm/include/llvm/Transforms/Utils/Cloning.h
index ae00c16e7eada..ec1a1d5faa7e9 100644
--- a/llvm/include/llvm/Transforms/Utils/Cloning.h
+++ b/llvm/include/llvm/Transforms/Utils/Cloning.h
@@ -230,20 +230,6 @@ void CloneAndPruneFunctionInto(Function *NewFunc, const Function *OldFunc,
const char *NameSuffix = "",
ClonedCodeInfo *CodeInfo = nullptr);
-/// Collect debug information such as types, compile units, and other
-/// subprograms that are reachable from \p F and can be considered global for
-/// the purposes of cloning (and hence not needing to be cloned).
-///
-/// What debug information should be processed depends on \p Changes: when
-/// cloning into the same module we process \p F's subprogram and instructions;
-/// when into a cloned module, neither of those.
-///
-/// Returns DISubprogram of the cloned function when cloning into the same
-/// module or nullptr otherwise.
-DISubprogram *CollectDebugInfoForCloning(const Function &F,
- CloneFunctionChangeType Changes,
- DebugInfoFinder &DIFinder);
-
/// This class captures the data input to the InlineFunction call, and records
/// the auxiliary results produced by it.
class InlineFunctionInfo {
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 249bef4696b8a..3ccb53236c026 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -168,27 +168,6 @@ void llvm::CloneFunctionAttributesInto(Function *NewFunc,
OldAttrs.getRetAttrs(), NewArgAttrs));
}
-DISubprogram *llvm::CollectDebugInfoForCloning(const Function &F,
- CloneFunctionChangeType Changes,
- DebugInfoFinder &DIFinder) {
- // CloneModule takes care of cloning debug info for ClonedModule. Cloning into
- // DifferentModule is taken care of separately in ClonedFunctionInto as part
- // of llvm.dbg.cu update.
- if (Changes >= CloneFunctionChangeType::DifferentModule)
- return nullptr;
-
- DISubprogram *SPClonedWithinModule = nullptr;
- if (Changes < CloneFunctionChangeType::DifferentModule) {
- SPClonedWithinModule = F.getSubprogram();
- }
- if (SPClonedWithinModule)
- DIFinder.processSubprogram(SPClonedWithinModule);
-
- collectDebugInfoFromInstructions(F, DIFinder);
-
- return SPClonedWithinModule;
-}
-
void llvm::CloneFunctionMetadataInto(Function &NewFunc, const Function &OldFunc,
ValueToValueMapTy &VMap,
RemapFlags RemapFlag,
>From 4e49c7f8ccf489039c3b70dbe82d044cdbb0c1f5 Mon Sep 17 00:00:00 2001
From: Artem Pianykh <arr at fb.com>
Date: Tue, 25 Feb 2025 13:07:40 -0800
Subject: [PATCH 5/5] [NFC][Cloning] Clean up comments in CloneFunctionInto
Summary:
Some comments no longer make sense nor refer to an existing code path.
Test Plan:
ninja check-llvm-unit
stack-info: PR: https://github.com/llvm/llvm-project/pull/129153, branch: users/artempyanykh/fast-coro-upstream-part2-take2/11
---
llvm/lib/Transforms/Utils/CloneFunction.cpp | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 3ccb53236c026..cde1ce8b43dbd 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -266,24 +266,13 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
if (OldFunc->isDeclaration())
return;
- // When we remap instructions within the same module, we want to avoid
- // duplicating inlined DISubprograms, so record all subprograms we find as we
- // duplicate instructions and then freeze them in the MD map. We also record
- // information about dbg.value and dbg.declare to avoid duplicating the
- // types.
DebugInfoFinder DIFinder;
- // Track the subprogram attachment that needs to be cloned to fine-tune the
- // mapping within the same module.
if (Changes < CloneFunctionChangeType::DifferentModule) {
- // Need to find subprograms, types, and compile units.
-
assert((NewFunc->getParent() == nullptr ||
NewFunc->getParent() == OldFunc->getParent()) &&
"Expected NewFunc to have the same parent, or no parent");
} else {
- // Need to find all the compile units.
-
assert((NewFunc->getParent() == nullptr ||
NewFunc->getParent() != OldFunc->getParent()) &&
"Expected NewFunc to have different parents, or no parent");
More information about the llvm-commits
mailing list