https://github.com/artempyanykh updated https://github.com/llvm/llvm-project/pull/129147
>From 17728ddd7c9e178978b02d693cb8b8806c1a331f Mon Sep 17 00:00:00 2001 From: Artem Pianykh <a...@fb.com> Date: Tue, 25 Feb 2025 10:53:57 -0800 Subject: [PATCH] [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 stack-info: PR: https://github.com/llvm/llvm-project/pull/129147, branch: users/artempyanykh/fast-coro-upstream-part2-take2/5 --- llvm/include/llvm/Transforms/Utils/Cloning.h | 4 +-- .../llvm/Transforms/Utils/ValueMapper.h | 27 ++++++++++--------- llvm/lib/Transforms/Coroutines/CoroSplit.cpp | 7 +++-- llvm/lib/Transforms/Utils/CloneFunction.cpp | 10 ++++--- llvm/lib/Transforms/Utils/ValueMapper.cpp | 15 +++++------ 5 files changed, 34 insertions(+), 29 deletions(-) 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 be35ef99261ed..b2c4e64319725 100644 --- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp @@ -921,11 +921,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 dd1b4fe718053..502c4898c5940 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 9882f81b759ac..5e50536a99206 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) {} @@ -904,11 +904,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"); @@ -1211,7 +1210,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); } _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
