[NFC][Cloning] Replace IdentityMD set with a predicate in ValueMapper #129147
+34
−29
Conversation
This file contains 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
Summary: We used the set only to check if it contains certain metadata nodes. Replacing the set with a predicate makes the intention clearer and the API more general. Test Plan: ninja check-all stack-info: PR: #129147, branch: users/artempyanykh/fast-coro-upstream-part2-take2/5
|
@llvm/pr-subscribers-coroutines @llvm/pr-subscribers-llvm-transforms Author: Artem Pianykh (artempyanykh) Changes[NFC][Cloning] Replace IdentityMD set with a predicate in ValueMapper Summary: Test Plan: Full diff: https://github.com/llvm/llvm-project/pull/129147.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/Cloning.h b/llvm/include/llvm/Transforms/Utils/Cloning.h
index d36f91416db88..2252dda0b9aad 100644
--- a/llvm/include/llvm/Transforms/Utils/Cloning.h
+++ b/llvm/include/llvm/Transforms/Utils/Cloning.h
@@ -194,7 +194,7 @@ void CloneFunctionMetadataInto(Function &NewFunc, const Function &OldFunc,
ValueToValueMapTy &VMap, RemapFlags RemapFlag,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr);
+ const MetadataPredicate *IdentityMD = nullptr);
/// Clone OldFunc's body into NewFunc.
void CloneFunctionBodyInto(Function &NewFunc, const Function &OldFunc,
@@ -204,7 +204,7 @@ void CloneFunctionBodyInto(Function &NewFunc, const Function &OldFunc,
ClonedCodeInfo *CodeInfo = nullptr,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr);
+ const MetadataPredicate *IdentityMD = nullptr);
void CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
const Instruction *StartingInst,
diff --git a/llvm/include/llvm/Transforms/Utils/ValueMapper.h b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
index 852d7095d1133..560df1d3f7f29 100644
--- a/llvm/include/llvm/Transforms/Utils/ValueMapper.h
+++ b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
@@ -37,6 +37,7 @@ class Value;
using ValueToValueMapTy = ValueMap<const Value *, WeakTrackingVH>;
using DbgRecordIterator = simple_ilist<DbgRecord>::iterator;
using MetadataSetTy = SmallPtrSet<const Metadata *, 16>;
+using MetadataPredicate = std::function<bool(const Metadata *)>;
/// This is a class that can be implemented by clients to remap types when
/// cloning constants and instructions.
@@ -138,8 +139,8 @@ inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) {
/// alternate \a ValueToValueMapTy and \a ValueMaterializer and returns a ID to
/// pass into the schedule*() functions.
///
-/// If an \a IdentityMD set is optionally provided, \a Metadata inside this set
-/// will be mapped onto itself in \a VM on first use.
+/// If an \a IdentityMD predicate is optionally provided, \a Metadata for which
+/// the predicate returns true will be mapped onto itself in \a VM on first use.
///
/// TODO: lib/Linker really doesn't need the \a ValueHandle in the \a
/// ValueToValueMapTy. We should template \a ValueMapper (and its
@@ -158,7 +159,7 @@ class ValueMapper {
ValueMapper(ValueToValueMapTy &VM, RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr);
+ const MetadataPredicate *IdentityMD = nullptr);
ValueMapper(ValueMapper &&) = delete;
ValueMapper(const ValueMapper &) = delete;
ValueMapper &operator=(ValueMapper &&) = delete;
@@ -225,7 +226,7 @@ inline Value *MapValue(const Value *V, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapValue(*V);
}
@@ -239,8 +240,8 @@ inline Value *MapValue(const Value *V, ValueToValueMapTy &VM,
/// \c MD.
/// 3. Else if \c MD is a \a ConstantAsMetadata, call \a MapValue() and
/// re-wrap its return (returning nullptr on nullptr).
-/// 4. Else if \c MD is in \c IdentityMD then add an identity mapping for it
-/// and return it.
+/// 4. Else if \c IdentityMD predicate returns true for \c MD then add an
+/// identity mapping for it and return it.
/// 5. Else, \c MD is an \a MDNode. These are remapped, along with their
/// transitive operands. Distinct nodes are duplicated or moved depending
/// on \a RF_MoveDistinctNodes. Uniqued nodes are remapped like constants.
@@ -251,7 +252,7 @@ inline Metadata *MapMetadata(const Metadata *MD, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapMetadata(*MD);
}
@@ -261,7 +262,7 @@ inline MDNode *MapMetadata(const MDNode *MD, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapMDNode(*MD);
}
@@ -278,7 +279,7 @@ inline void RemapInstruction(Instruction *I, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.remapInstruction(*I);
}
@@ -289,7 +290,7 @@ inline void RemapDbgRecord(Module *M, DbgRecord *DR, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.remapDbgRecord(M, *DR);
}
@@ -302,7 +303,7 @@ inline void RemapDbgRecordRange(Module *M,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.remapDbgRecordRange(M, Range);
}
@@ -317,7 +318,7 @@ inline void RemapFunction(Function &F, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD).remapFunction(F);
}
@@ -326,7 +327,7 @@ inline Constant *MapValue(const Constant *V, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr,
- const MetadataSetTy *IdentityMD = nullptr) {
+ const MetadataPredicate *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapConstant(*V);
}
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index e1f767edd6ee1..8179d5c0b29b5 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -922,11 +922,14 @@ 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,
- &CommonDebugInfo);
+ &IdentityMD);
CloneFunctionBodyInto(*NewF, OrigF, VMap, RF_None, Returns, "", nullptr,
- nullptr, nullptr, &CommonDebugInfo);
+ nullptr, nullptr, &IdentityMD);
auto &Context = NewF->getContext();
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 6b6315d698c9a..8e2a02d49d35c 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -204,7 +204,7 @@ void llvm::CloneFunctionMetadataInto(Function &NewFunc, const Function &OldFunc,
RemapFlags RemapFlag,
ValueMapTypeRemapper *TypeMapper,
ValueMaterializer *Materializer,
- const MetadataSetTy *IdentityMD) {
+ const MetadataPredicate *IdentityMD) {
SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
OldFunc.getAllMetadata(MDs);
for (auto MD : MDs) {
@@ -221,7 +221,7 @@ void llvm::CloneFunctionBodyInto(Function &NewFunc, const Function &OldFunc,
ClonedCodeInfo *CodeInfo,
ValueMapTypeRemapper *TypeMapper,
ValueMaterializer *Materializer,
- const MetadataSetTy *IdentityMD) {
+ const MetadataPredicate *IdentityMD) {
if (OldFunc.isDeclaration())
return;
@@ -328,8 +328,10 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
DISubprogram *SPClonedWithinModule =
CollectDebugInfoForCloning(*OldFunc, Changes, DIFinder);
- MetadataSetTy IdentityMD =
- FindDebugInfoToIdentityMap(Changes, DIFinder, SPClonedWithinModule);
+ MetadataPredicate IdentityMD =
+ [MDSet =
+ FindDebugInfoToIdentityMap(Changes, DIFinder, SPClonedWithinModule)](
+ const Metadata *MD) { return MDSet.contains(MD); };
// Cloning is always a Module level operation, since Metadata needs to be
// cloned.
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index b8569454379bf..7082dfb311ffc 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -120,12 +120,12 @@ class Mapper {
SmallVector<WorklistEntry, 4> Worklist;
SmallVector<DelayedBasicBlock, 1> DelayedBBs;
SmallVector<Constant *, 16> AppendingInits;
- const MetadataSetTy *IdentityMD;
+ const MetadataPredicate *IdentityMD;
public:
Mapper(ValueToValueMapTy &VM, RemapFlags Flags,
ValueMapTypeRemapper *TypeMapper, ValueMaterializer *Materializer,
- const MetadataSetTy *IdentityMD)
+ const MetadataPredicate *IdentityMD)
: Flags(Flags), TypeMapper(TypeMapper),
MCs(1, MappingContext(VM, Materializer)), IdentityMD(IdentityMD) {}
@@ -901,11 +901,10 @@ std::optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
return wrapConstantAsMetadata(*CMD, mapValue(CMD->getValue()));
}
- // Map metadata from IdentityMD on first use. We need to add these nodes to
- // the mapping as otherwise metadata nodes numbering gets messed up. This is
- // still economical because the amount of data in IdentityMD may be a lot
- // larger than what will actually get used.
- if (IdentityMD && IdentityMD->contains(MD))
+ // Map metadata matching IdentityMD predicate on first use. We need to add
+ // these nodes to the mapping as otherwise metadata nodes numbering gets
+ // messed up.
+ if (IdentityMD && (*IdentityMD)(MD))
return getVM().MD()[MD] = TrackingMDRef(const_cast<Metadata *>(MD));
assert(isa<MDNode>(MD) && "Expected a metadata node");
@@ -1208,7 +1207,7 @@ class FlushingMapper {
ValueMapper::ValueMapper(ValueToValueMapTy &VM, RemapFlags Flags,
ValueMapTypeRemapper *TypeMapper,
ValueMaterializer *Materializer,
- const MetadataSetTy *IdentityMD)
+ const MetadataPredicate *IdentityMD)
: pImpl(new Mapper(VM, Flags, TypeMapper, Materializer, IdentityMD)) {}
ValueMapper::~ValueMapper() { delete getAsMapper(pImpl); }
|
artempyanykh
force-pushed
the
users/artempyanykh/fast-coro-upstream-part2-take2/5
branch
from
c24bd57 to
9675cd2
Compare
artempyanykh
force-pushed
the
users/artempyanykh/fast-coro-upstream-part2-take2/4
branch
from
90deeb1 to
d02c645
Compare
This was referenced Feb 27, 2025
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.
Stacked PRs:
[NFC][Cloning] Replace IdentityMD set with a predicate in ValueMapper
Summary:
We used the set only to check if it contains certain metadata nodes.
Replacing the set with a predicate makes the intention clearer and the
API more general.
Test Plan:
ninja check-all