Skip to content

[NFC][Utils] Extract CloneFunctionMetadataInto from CloneFunctionInto #118623

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

artempyanykh
Copy link
Contributor

@artempyanykh artempyanykh commented Dec 4, 2024

@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Artem Pianykh (artempyanykh)

Changes

[NFC][Utils] Extract CloneFunctionMetadataInto from CloneFunctionInto

Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm


Full diff: https://github.com/llvm/llvm-project/pull/118623.diff

2 Files Affected:

  • (modified) llvm/include/llvm/Transforms/Utils/Cloning.h (+12)
  • (modified) llvm/lib/Transforms/Utils/CloneFunction.cpp (+19-9)
diff --git a/llvm/include/llvm/Transforms/Utils/Cloning.h b/llvm/include/llvm/Transforms/Utils/Cloning.h
index 434089138bc521..698d773525e80b 100644
--- a/llvm/include/llvm/Transforms/Utils/Cloning.h
+++ b/llvm/include/llvm/Transforms/Utils/Cloning.h
@@ -182,6 +182,18 @@ void CloneFunctionAttributesInto(Function *NewFunc, const Function *OldFunc,
                                  ValueMapTypeRemapper *TypeMapper = nullptr,
                                  ValueMaterializer *Materializer = nullptr);
 
+/// Clone OldFunc's metadata into NewFunc.
+///
+/// The caller is expected to populate \p VMap beforehand and set an appropriate
+/// \p RemapFlag.
+///
+/// NOTE: This function doesn't clone !llvm.dbg.cu when cloning into a different
+/// module. Use CloneFunctionInto for that behavior.
+void CloneFunctionMetadataInto(Function *NewFunc, const Function *OldFunc,
+                               ValueToValueMapTy &VMap, RemapFlags RemapFlag,
+                               ValueMapTypeRemapper *TypeMapper = nullptr,
+                               ValueMaterializer *Materializer = nullptr);
+
 void CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
                                const Instruction *StartingInst,
                                ValueToValueMapTy &VMap, bool ModuleLevelChanges,
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 736c4a13045c14..4306e5d2abdd18 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -199,6 +199,22 @@ bool llvm::BuildDebugInfoMDMap(MDMapT &MD, CloneFunctionChangeType Changes,
   return ModuleLevelChanges;
 }
 
+void llvm::CloneFunctionMetadataInto(Function *NewFunc, const Function *OldFunc,
+                                     ValueToValueMapTy &VMap,
+                                     RemapFlags RemapFlag,
+                                     ValueMapTypeRemapper *TypeMapper,
+                                     ValueMaterializer *Materializer) {
+  // Duplicate the metadata that is attached to the cloned function.
+  // Subprograms/CUs/types that were already mapped to themselves won't be
+  // duplicated.
+  SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
+  OldFunc->getAllMetadata(MDs);
+  for (auto MD : MDs) {
+    NewFunc->addMetadata(MD.first, *MapMetadata(MD.second, VMap, RemapFlag,
+                                                TypeMapper, Materializer));
+  }
+}
+
 // Clone OldFunc into NewFunc, transforming the old arguments into references to
 // VMap values.
 void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
@@ -261,15 +277,9 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
       BuildDebugInfoMDMap(VMap.MD(), Changes, DIFinder, SPClonedWithinModule);
 
   const auto RemapFlag = ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges;
-  // Duplicate the metadata that is attached to the cloned function.
-  // Subprograms/CUs/types that were already mapped to themselves won't be
-  // duplicated.
-  SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
-  OldFunc->getAllMetadata(MDs);
-  for (auto MD : MDs) {
-    NewFunc->addMetadata(MD.first, *MapMetadata(MD.second, VMap, RemapFlag,
-                                                TypeMapper, Materializer));
-  }
+
+  CloneFunctionMetadataInto(NewFunc, OldFunc, VMap, RemapFlag, TypeMapper,
+                            Materializer);
 
   // Loop over all of the basic blocks in the function, cloning them as
   // appropriate.  Note that we save BE this way in order to handle cloning of

@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/3 branch from bd9ed1a to b968b21 Compare December 4, 2024 13:16
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/4 branch from 4c89772 to 36b917e Compare December 4, 2024 13:16
jollaitbot pushed a commit to sailfishos-mirror/llvm-project that referenced this pull request Dec 4, 2024
Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm

stack-info: PR: llvm/llvm-project#118623, branch: users/artempyanykh/fast-coro-upstream/4
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/3 branch from b968b21 to 69aad05 Compare December 6, 2024 12:42
artempyanykh added a commit that referenced this pull request Dec 6, 2024
Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm

stack-info: PR: #118623, branch: users/artempyanykh/fast-coro-upstream/4
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/4 branch from 36b917e to 8540f25 Compare December 6, 2024 12:42
@artempyanykh artempyanykh changed the base branch from users/artempyanykh/fast-coro-upstream/3 to main December 6, 2024 14:03
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/4 branch from 8540f25 to ca3fb5e Compare December 6, 2024 14:04
@artempyanykh artempyanykh changed the base branch from main to users/artempyanykh/fast-coro-upstream/3 December 6, 2024 14:04
jollaitbot pushed a commit to sailfishos-mirror/llvm-project that referenced this pull request Dec 6, 2024
Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm

stack-info: PR: llvm/llvm-project#118623, branch: users/artempyanykh/fast-coro-upstream/4
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/3 branch from 8b5b048 to 6e46c39 Compare December 9, 2024 12:40
artempyanykh added a commit that referenced this pull request Dec 9, 2024
Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm

stack-info: PR: #118623, branch: users/artempyanykh/fast-coro-upstream/4
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/4 branch from ca3fb5e to 8ce25ca Compare December 9, 2024 12:40
@artempyanykh artempyanykh changed the base branch from users/artempyanykh/fast-coro-upstream/3 to main December 9, 2024 16:57
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/4 branch from 8ce25ca to 9dcfc56 Compare December 9, 2024 16:57
@artempyanykh artempyanykh changed the base branch from main to users/artempyanykh/fast-coro-upstream/3 December 9, 2024 16:57
artempyanykh added a commit to artempyanykh/llvm-project that referenced this pull request Dec 9, 2024
Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm

stack-info: PR: llvm#118623, branch: users/artempyanykh/fast-coro-upstream/4
///
/// NOTE: This function doesn't clone !llvm.dbg.cu when cloning into a different
/// module. Use CloneFunctionInto for that behavior.
void CloneFunctionMetadataInto(Function *NewFunc, const Function *OldFunc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can NewFunc or OldFunc be null? If not, we should make them references

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both can't be null. Made them references.

RemapFlags RemapFlag,
ValueMapTypeRemapper *TypeMapper,
ValueMaterializer *Materializer) {
// Duplicate the metadata that is attached to the cloned function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that, in the context of this function, there is no "cloned function", it's either NewFunc or OldFunc.

That said, I this comment fits better in the documentation of the header, as there is important information here that callers should be aware

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Moved relevant documentation bits into Cloning.h.

Base automatically changed from users/artempyanykh/fast-coro-upstream/3 to main December 10, 2024 08:10
artempyanykh added a commit that referenced this pull request Dec 10, 2024
Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm

stack-info: PR: #118623, branch: users/artempyanykh/fast-coro-upstream/4
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/4 branch from 9dcfc56 to cec0213 Compare December 10, 2024 08:10
Summary:
The new API expects the caller to populate the VMap. We need it this way
for a subsequent change around coroutine cloning.

Test Plan:
ninja check-llvm-unit check-llvm

stack-info: PR: #118623, branch: users/artempyanykh/fast-coro-upstream/4
@artempyanykh artempyanykh force-pushed the users/artempyanykh/fast-coro-upstream/4 branch from cec0213 to a91e783 Compare December 11, 2024 07:48
@artempyanykh artempyanykh merged commit a9237b1 into main Dec 16, 2024
8 checks passed
@artempyanykh artempyanykh deleted the users/artempyanykh/fast-coro-upstream/4 branch December 16, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants